[Short Backgroung]
After the introduction of the "Graphics View" framework to Qt (since version 4.2), managing and interacting with a large number of custom 2D based graphical items is simple and fast in Qt. As a summary, there are three major entities involved:
- QGraphicsScene is a container for QGraphicsItem objects (widgets).
- QGraphicsView provides the view widget, which visualizes the content of a scene.
- QGraphicsItem is the base class for graphical items in a scene.
In order to embed non-QGraphicsItem based objects ( e.g. most of QWidget subclasses ) into a scene, a "proxy" widget (QGraphicsProxyWidget) is necessary, which can represent a pontential performance bottle-neck to applications, specially on mobile devices.
[Bug report]
This bug is about providing a QGraphicsItem version of the "QWebView" widget, making it to be a straightforward embeddable widget into Graphics View based applications.

There are many other points that shows that a GraphicsItem based widget would be a win for QtWebKit. Many different teams, people have already implemented this, such as one for QML, one for Plasma, etc.
I have been told today that the KDE team is very interested in this work, and would like to use it in their new Silk project.
It should also be noted that using the proxy widget will result in full update of the viewport for each scroll operation, something not desirable, especially on slow, embedded devices.

As discussed with Kenneth briefly in chat, I think it would make sense to get this class reviewed and landed in a basic form. Even without optimizations it's already of big use and help and it would form a good basis allowing other teams to contribute their optimizations or features for better graphicsview support.

In the beginning(In reply to comment #4)
> Kenneth, why is it a QGraphicsWidget instead of a QGraphicsObject?
In the beginning it was a QGraphicsLayoutItem and a QGraphicsObject, but it made a whole lot more sense making it a QGraphicsWidget as I was basically implementing the same things as it already provided.
It should be noted that QGraphicsWidget imherits QGraphicsObject and QGraphicsLayoutItem

Naming concern: all graphics item use the format QGraphicsFooItem, even QGraphicsSvgItem which is part of QtSvg (not QtGui). But then, in QtWebKit we always use QWebFoo moniker. So what is our choice here?
The API and the code look clean and mimic QWebView enough that developers will not have problem switching from one to another.
Since we deprecate textSizeMultiplier in QWebView, should we also remove it here?
Question: if someone wants to add flick support, is it possible with the current code? Or does this need to be handled in a higher level (think Declarative UI with FxFlickable)?

(In reply to comment #8)
> Naming concern: all graphics item use the format QGraphicsFooItem, even
> QGraphicsSvgItem which is part of QtSvg (not QtGui). But then, in QtWebKit we
> always use QWebFoo moniker. So what is our choice here?
This was already discussed with Simon some time ago, and since it is part of the QtWebKit module it should start with QWeb*, hense the current name.
> Since we deprecate textSizeMultiplier in QWebView, should we also remove it
> here?
If that is the case, I'm all for it.
> Question: if someone wants to add flick support, is it possible with the
> current code? Or does this need to be handled in a higher level (think
> Declarative UI with FxFlickable)?
I would like to add that support to the QGVLauncher as a menu option, so we should find out soon if that works :-)

Antonio, this patch depends on my patch that adds support of handling QGraphicsScene event in QWebPage. Can you attach that one? or I will do it when I am at work Tuesday.
Also, there is no patch attached to the other bugzilla entry (28865).

(In reply to comment #11)
> Antonio, this patch depends on my patch that adds support of handling
> QGraphicsScene event in QWebPage.
in fact, i'd say they are complementary ...
> Also, there is no patch attached to the other bugzilla entry (28865).
it is there now.

Comment on attachment 39125[details]
patch 2 - v0.1 - Add support for handling QGraphicsScene events.
In principle this approach is good, QWebPage _should_ become capable of dealing with QGraphicsScene events.
However this implementation duplicates a some of the existing event handlers (drag'n'drop) and it converts
the graphicsview events to QWidget events and then calls the original handlers. This means a lot of unnecessary
object creation.
I think it would be better to change the existing handlers, for example QWebPagePrivate::mousePressEvent, to
take a PlatformMouseEvent as argument. Then PlatformMouseEvent should be extended with a constructor that
accepts a QGraphicsSceneMouseEvent. This way the code in QWebPage::event() takes care of just calling the
right constructors, the PlatformFooEvent constructors take care of extracting the parameters and the logic
in QWebPagePrivate's event handlers can remain without the need of duplication.

I totally agree with you. Actually I was going to make that another follow up patch, as I wanted to do it iteratively.
Would that be possible? or do you want me to fix the patch as is?
> However this implementation duplicates a some of the existing event handlers
> (drag'n'drop) and it converts
> the graphicsview events to QWidget events and then calls the original handlers.
> This means a lot of unnecessary
> object creation.

> we were getting viewport doabled updated in scroll events. That is the way
> Kenneth found to avoid that. i will let to kenneth a more detailed answer here,
> if needed.
Actually the problem is that it is doing implicit updates whenever someone sets the view on the page. For instance if someone does that with the viewport of the graphicsview it will receive wrong updates, and scrolling will be very broken. I do not think it is desirable to let the user be able to do so, so I restricted the *implicit* updates to the QWebPage. That seemed like the right thing to do.
Earlier I had another patch allowing the QWebGrapicsItem to privately disable implicit updates.
Any reason you want to allow implicit paints on other kind of view than QWebPage? If so that should definately be documented in the setView method.

Comment on attachment 39165[details]
patch 1 - v0.2.1 - add QWebGraphicsItem to the API and QGVLauncher sample application
> +#include "qwebpage_p.h"
Is this really needed?
> +#include <QGraphicsScene>
> +#include <QGraphicsView>
Can we use the <QtGui/Foo> pattern also here?
> + setFlag(QGraphicsItem::ItemUsesExtendedStyleOption, true);
This is Qt 4.6 only. Please guard it with QT_VERSION properly for <= 4.5.
> + if (this->progress == progress / 100.0)
Please use qFuzzyCompare.
> +void QWebGraphicsItemPrivate::_q_doScroll(int dx, int dy, const QRect& rectToScroll)
> +{
> + q->scroll(qreal(dx), qreal(dy), QRectF(rectToScroll));
> +}
I found out that this does not really work if I subclass QWebGraphicsItem.
An example: I have my customized QWebGraphicsItem that clips the painting so that the corners are rounded. When this function is invoked, say scrolling vertically, the whole painted item is scrolled, meaning the bottom rounded corners are also scrolled. The correct thing of course that the _contents_ of the web page is scrolled, not merely the item.
The real fix would be to call update, instead of scroll. I understand that this is optimization issue, but I'd rather have a correctly painted item than a fast but garbage one.
> +bool QWebGraphicsItem::interactive() const
Following Qt-ish naming, this should be isInteractive instead.
> + emit interactiveChanged();
The name could be misleading. What about interactivityChanged?
> +++ b/WebKit/qt/Api/qwebgraphicsitem_p.h
Is there any reason we need separate file for this? Unline other qweb* classes, QWebGraphicsItemPrivate is not need by anyone else, right?
Overall, looks really good!
r- until the issues above are addressed.

I'd like to suggest the following coarse of action:
Rather than assuming that the viewer of a web page is a particular kind of
class, be it QWebView, QWidget, QGraphicsSomething, etc, lets do this:
replace
QWidget* view()
with
QSomeCoolSoundingName *view()
where
class QSomeCoolSoundingName:public QObject
{
public:
virtual QRect windowRect();
virtual void focus();
virtual void unfocus();
virtual void show();
virtual void repaint(const QRect& windowRect, bool contentChanged, bool
immediate, bool repaintContentOnly);
virtual void scroll(const QRect& delta, const QRect& scrollViewRect, const
QRect& clipRect);
virtual void setToolTip(const String &tip);
//BIG mystery here:
virtual QWidget* platformWindow() const;
};
then a QWebPage gets a pointer to a view() object. When one uses QtWebKit, then
one provides such an object whose implementation one writes for one's way of
displaying and handling display. This way those insisting on QWidget can be
supported via old style QWebView, newer implementations can have
QGraphicsWebView, and others can customize any way they like subject to their
needs.
With the exception to platformWindow(), all of the above are quite obvious on
what to write if one is using a QGraphicsSomething to draw, or even to a
QPixmap as part of a something else.
The biggest offender is ChromeClientQt.cpp, but there are some others:
DragClientQt.cpp, EditorClientQt.cpp, FrameLoaderClientQt.cpp,
InspectorClientQt.cpp
I am worried about what happens with plugins, and this is where careful
attention needs to be taken care in deciding how to handle platformWindow().

> replace
> QWidget* view()
>
> with
>
> QSomeCoolSoundingName *view()
This changes is binary incompatible.
If we want to introduce other function for that, we end up having two "views". Which one to use?

(In reply to comment #23)
> I'd like to suggest the following coarse of action:
>
> Rather than assuming that the viewer of a web page is a particular kind of
> class, be it QWebView, QWidget, QGraphicsSomething, etc, lets do this:
>
> replace
> QWidget* view()
>
> with
>
> QSomeCoolSoundingName *view()
>
> where
>
> class QSomeCoolSoundingName:public QObject
> {
> public:
> virtual QRect windowRect();
> virtual void focus();
> virtual void unfocus();
> virtual void show();
> virtual void repaint(const QRect& windowRect, bool contentChanged, bool
> immediate, bool repaintContentOnly);
> virtual void scroll(const QRect& delta, const QRect& scrollViewRect, const
> QRect& clipRect);
> virtual void setToolTip(const String &tip);
>
> //BIG mystery here:
> virtual QWidget* platformWindow() const;
>
> };
>
> then a QWebPage gets a pointer to a view() object. When one uses QtWebKit, then
> one provides such an object whose implementation one writes for one's way of
> displaying and handling display. This way those insisting on QWidget can be
> supported via old style QWebView, newer implementations can have
> QGraphicsWebView, and others can customize any way they like subject to their
> needs.
>
> With the exception to platformWindow(), all of the above are quite obvious on
> what to write if one is using a QGraphicsSomething to draw, or even to a
> QPixmap as part of a something else.
>
> The biggest offender is ChromeClientQt.cpp, but there are some others:
> DragClientQt.cpp, EditorClientQt.cpp, FrameLoaderClientQt.cpp,
> InspectorClientQt.cpp
It sounds okay to me to introduce this class, but I would prefer to keep it internal and not make it part of the public API initially. It seems more sensible to me to utilize it as internal interface first to abstract away the difference between QWebGraphicsItem and QWebView.

> > +void QWebGraphicsItemPrivate::_q_doScroll(int dx, int dy, const QRect& rectToScroll)
> > +{
> > + q->scroll(qreal(dx), qreal(dy), QRectF(rectToScroll));
> > +}
>
> I found out that this does not really work if I subclass QWebGraphicsItem.
> An example: I have my customized QWebGraphicsItem that clips the painting so
> that the corners are rounded. When this function is invoked, say scrolling
> vertically, the whole painted item is scrolled, meaning the bottom rounded
> corners are also scrolled. The correct thing of course that the _contents_ of
> the web page is scrolled, not merely the item.
> The real fix would be to call update, instead of scroll. I understand that this
> is optimization issue, but I'd rather have a correctly painted item than a fast
> but garbage one.
According to Alexis, you are supposed to reimplement
QPainterPath QGraphicsItem::shape () const [virtual]
to not return a rectangular shape anymore, but the shape of the item with clipping. This is also used for events (the clipped area should be able to receive events), collision detection, and the QGraphicsScene::items() function.
If thsi doesn't work, it can be considered a graphics view bug, I suspect.

After a discussion with Simon we've come up with a strategy for the future relationship/interface between QWebPage and QWebView et. al.
Basically we want the interface between the QWebPage and any clients to be as clean and decoupled as possible, while still giving us freedom to do stuff like the cursor changes.
Right now the QWebPage interface is mostly in the form of a model in MVC terms, where we have signals (and setters and getters) for changes in the page. We have some hooks where we go directly to the QWebView, which of course increases coupling.
What we propose is that we add a private client interface, QWebPageClient or something, that QWebViewPrivate et al. will implement. The QWebPage will keep a pointer to a client, and WebCore will pass on callbacks to the client for stuff like the cursor change. (Side note: the cursor patch in bug 28865 should follow this pattern in a future update, ie not use the dynamic property approach, but that's OK for now).
As we see callbacks mature and stabilize in the private QWebPageClient we can move them to the public QWebPage API by transforming into a proper decoupled clean signal+getter/setter-interface, so any features/callbacks in the QWebPageClient should have this in mind (ie. prepare for the future).
I think this will give us a good mix of a stable, well tested and decoupled QWebPage API, while at the same time giving us the chance to do cool stuff for QWebView and QWebGraphicsItem.

(In reply to comment #22)
> > +void QWebGraphicsItemPrivate::_q_doScroll(int dx, int dy, const QRect& rectToScroll)
> > +{
> > + q->scroll(qreal(dx), qreal(dy), QRectF(rectToScroll));
> > +}
>
> I found out that this does not really work if I subclass QWebGraphicsItem.
> An example: I have my customized QWebGraphicsItem that clips the painting so
> that the corners are rounded. When this function is invoked, say scrolling
> vertically, the whole painted item is scrolled, meaning the bottom rounded
> corners are also scrolled. The correct thing of course that the _contents_ of
> the web page is scrolled, not merely the item.
> The real fix would be to call update, instead of scroll. I understand that this
> is optimization issue, but I'd rather have a correctly painted item than a fast
> but garbage one.
Hehe, well, being able to _scroll_ the webpage instead of repainting it entirely is one of the primary purposes of QWebGraphicsItem. With a QWebView embedded using a QGraphicsProxyWidget we get exactly that repaint-on-scroll behaviour that makes it so slow to use.
I agree with Kenneth that this is either a QGraphicsView bug or we lack a mechanism to determine if it is safe to scroll or not. In the worst case we make it a property of QWebGraphicsItem (scrollMode?), in the best case we can detect this situation automatically, similar to QGraphicsItem::scroll()'s implementation.

> Hehe, well, being able to _scroll_ the webpage instead of repainting it
> entirely is one of the primary purposes of QWebGraphicsItem. With a QWebView
> embedded using a QGraphicsProxyWidget we get exactly that repaint-on-scroll
> behaviour that makes it so slow to use.
>
> I agree with Kenneth that this is either a QGraphicsView bug or we lack a
> mechanism to determine if it is safe to scroll or not. In the worst case we
> make it a property of QWebGraphicsItem (scrollMode?), in the best case we can
> detect this situation automatically, similar to QGraphicsItem::scroll()'s
> implementation.
Actually if the shape() doesn't work, we could use it in _q_doScroll to get the inner rect and restrict the scroll area to it, and then call update() for the other areas.

> Overall, looks really good!
> r- until the issues above are addressed.
ariya, thanks for your comments.
(In reply to comment #22)
> (From update of attachment 39165[details])
>
> > +#include "qwebpage_p.h"
> Is this really needed?
so far, yes ... since we are doing this in dtror:
QWebGraphicsItem::~QWebGraphicsItem()
{
if (d->page)
d->page->d->view = 0;
(..)
> > +#include <QGraphicsScene>
> > +#include <QGraphicsView>
> Can we use the <QtGui/Foo> pattern also here?
done
> > + setFlag(QGraphicsItem::ItemUsesExtendedStyleOption, true);
> This is Qt 4.6 only. Please guard it with QT_VERSION properly for <= 4.5.
done
> > + if (this->progress == progress / 100.0)
> Please use qFuzzyCompare.
done
> > +bool QWebGraphicsItem::interactive() const
> Following Qt-ish naming, this should be isInteractive instead.
> > + emit interactiveChanged();
done
> The name could be misleading. What about interactivityChanged?
> > +++ b/WebKit/qt/Api/qwebgraphicsitem_p.h
> Is there any reason we need separate file for this? Unline other qweb* classes, QWebGraphicsItemPrivate is not need by anyone else, right?
ok, i removed _p.h
> > +void QWebGraphicsItemPrivate::_q_doScroll(int dx, int dy, const QRect& rectToScroll)
> > +{
> > + q->scroll(qreal(dx), qreal(dy), QRectF(rectToScroll));
> > +}
>
> I found out that this does not really work if I subclass QWebGraphicsItem.
> An example: I have my customized QWebGraphicsItem that clips the painting so
> that the corners are rounded. When this function is invoked, say scrolling
> vertically, the whole painted item is scrolled, meaning the bottom rounded
> corners are also scrolled. The correct thing of course that the _contents_ of
> the web page is scrolled, not merely the item.
> The real fix would be to call update, instead of scroll. I understand that this is optimization issue, but I'd rather have a correctly painted item than a fast but garbage one.
as per discussed w/ kenneth and others, I would rather let this issue to be fixed in a followup bug if possible, as well as some other changes we are listing here:
1) PageClient class (as well as make up a way to platformWidget to not return a QWidget necessarily) - see comment #27
2) make cursor to work
3) Scroll optimization

Returning to my idea posted as (In reply to comment #25)
> It sounds okay to me to introduce this class, but I would prefer to keep it
> internal and not make it part of the public API initially. It seems more
> sensible to me to utilize it as internal interface first to abstract away the
> difference between QWebGraphicsItem and QWebView.
Actually, I'd like to make this _public_. Because right now there is a hassle in the moving from QWidget to QGraphicsThingy, and in the future the view object might change again and there are some use cases where the viewer is not a QWidget or QGraphicsThingy but wants the implicit updates from WebCore and the ability to optimize for scroll, etc.
As for binary compatibility, it is NOT binary compatible, but if we take a little care it can atleast be source compatible with the old style QWebView usage. I am totally against having to support two different "view()'s ", rather replace the QWidget with the new dumby class that a user writes and we could also provide 3 public implementations of that class:
1. where view() hooks into a QWebView
2. where view() hooks into a "QGraphicsView"
3. where view() hooks into a QPixmap, i.e. the derived object will have a pointer to a QPixmap to draw too.
This way, useful examples (in both showing how and actually useful by themselves) are exposed.

(In reply to comment #35)
> Returning to my idea posted as (In reply to comment #25)
>
> > It sounds okay to me to introduce this class, but I would prefer to keep it
> > internal and not make it part of the public API initially. It seems more
> > sensible to me to utilize it as internal interface first to abstract away the
> > difference between QWebGraphicsItem and QWebView.
>
> Actually, I'd like to make this _public_. Because right now there is a hassle
> in the moving from QWidget to QGraphicsThingy, and in the future the view
> object might change again and there are some use cases where the viewer is not
> a QWidget or QGraphicsThingy but wants the implicit updates from WebCore and
> the ability to optimize for scroll, etc.
>
> As for binary compatibility, it is NOT binary compatible, but if we take a
> little care it can atleast be source compatible with the old style QWebView
> usage. I am totally against having to support two different "view()'s ", rather
> replace the QWidget with the new dumby class that a user writes and we could
> also provide 3 public implementations of that class:
>
> 1. where view() hooks into a QWebView
> 2. where view() hooks into a "QGraphicsView"
> 3. where view() hooks into a QPixmap, i.e. the derived object will have a
> pointer to a QPixmap to draw too.
>
> This way, useful examples (in both showing how and actually useful by
> themselves) are exposed.
Unfortunately if it doesn't remain binary compatible it cannot be part of the public API. That's exactly the issue, and that's also why Tor Arne suggested in an earlier comment that we should use QWebPageClient as a staging area for API that should become public once we're confident that it's a reasonable thing to have in a view of QWebPage.

Landed in 48219.
> Please also re-implement QGraphicsItem::sceneEvent() as well as
> QObject::event(), similar to QWebView.
> That allows fixing event-related bugs in patch releases without the need to
> re-implement an existing
> virtual function, which by adding a new symbol cannot be done in patch releases
> otherwise.
OK, will do it a follow up.
> > +++ b/WebKit/qt/QGVLauncher/main.cpp
>
> This file has a fewcoding style violations in them (* placement for example).
> Please fix before landing.
Fixed.
> > + // Only do implicit paints for QWebView's
> > + if (QWebView* view = qobject_cast<QWebView*>(m_webPage->view()))> I really hope that we can eliminate these casts in the future, for example by
> making
> QWebGraphicsItem not call setView() but instead simply keep the widget from the
> events
> around but not using it in the above methods.
>
With the QWebPageClient we will be able to do so.

+/*!
+ \property QWebGraphicsItem::zoomFactor
+ \since 4.5
+ \brief the zoom factor for the view
+*/
The \since tag should be removed.
+/*!
+ Returns a pointer to the view's history of navigated web pages.
+
+ It is equivalent to
+
+ \snippet webkitsnippets/qtwebkit_qwebview_snippet.cpp 0
The snippet is for QWebView, should be removed or we should have another one based on QWebGraphicsItem.
+QWebHistory* QWebGraphicsItem::history() const
+{
+ return d->page->history();
+}
A bunch of functions such as settings(), load(), history() would crash when the page is null, they should either construct a QWebPage or silently return. An autotest would catch it, I can help you out with that. :)
Very nice, Kenneth. :)

Comment on attachment 39299[details]
Followup: Implement some virtual event methods so that we can fix event-related bugs in Qt patch releases
Cool stuff, minor nitpick:
> + // Swallow reimplementation in order to allows fixing event-related bugs in patch releases
"Swallow"?
I think you can drop the comment, or at least fix the "allows" spelling and say something like "Re-implemented in order ..."

> Unfortunately if it doesn't remain binary compatible it cannot be part of the
> public API. That's exactly the issue, and that's also why Tor Arne suggested in
> an earlier comment that we should use QWebPageClient as a staging area for API
> that should become public once we're confident that it's a reasonable thing to
> have in a view of QWebPage.
You are right, and the QWebPageClient is the right thing to do, but, what I do not see how to do is to keep binary compatibility since the files under qt/WebCoreSupport/ are all thinking "QWidget/QWebView" and to keep binary compatibility would they have to support both QWebView and QWebPageClient with preference to QWebPageClient?

(In reply to comment #51)
> I'm starting to think we should close this bug and continue with new bugs on
> follow-up issues, including QWebPageClient.
agreed.
guys, please lets move discussion about WebPageClient design/impl/gols to bug 29085 , and spin current bug off into followup bug for remaining issues.

(In reply to comment #54)
> some follow up bugs:
> * Bug 29155 - [Qt] Implement autotests for QWebGraphicsItem
> * Bug 29156 - [Qt] Fix scrolling implementation on QWebGraphicsItem
That is a bit misleading, as the Oslo guys confirmed that our scrolling implementation is indeed correct and that when subclassing you need to implement the virtual shape function as well.
If that isn't working, that is a GraphicsView bug and not a WebKit one.