|
Description
Antonio Gomes
2009-08-31 19:38:29 PDT
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. For reference, Kenneth's development branch can be found at http://www.gitorious.org/~kenneth/qtwebkit/kenneth-devel/commits/graphicsitem 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. Kenneth, why is it a QGraphicsWidget instead of a QGraphicsObject? FYI, link to the current header file: http://www.gitorious.org/~kenneth/qtwebkit/kenneth-devel/blobs/graphicsitem/WebKit/qt/Api/qwebgraphicsitem.h 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 Is there any reason for discouraging the usage of QGraphicsWidget? It seems to fit our role pretty well. 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 :-) Created attachment 39117 [details] patch 1 - v0.1 - add QWebGraphicsItem to the API and QGVLauncher sample application. - Add QWebGraphicsItem to the API. - QGVLauncher as a sample. - Change the build system accordingly. * Known isues: cursor depends on a patch from bug 28865. 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). Created attachment 39125 [details]
patch 2 - v0.1 - Add support for handling QGraphicsScene events.
Add support for handling QGraphicsScene events.
(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 39117 [details] patch 1 - v0.1 - add QWebGraphicsItem to the API and QGVLauncher sample application. Looks good to me in principle, just a few minor things that should be fixed IMHO before landing. > * Api/headers.pri: > * Api/qwebgraphicsitem.cpp: Added. > (QWebGraphicsItem::QWebGraphicsItem): > (QWebGraphicsItem::~QWebGraphicsItem): [...lots of functions here...] I think you can leave out the long list of functions from the changelog, it's just bloat :) > +/*! > + \brief The WebPage item allows you to add web content to a canvas. > + \inherits QGraphicsWidget. Please add a \since 4.6 tag here > +void QWebGraphicsItem::paint(QPainter* painter, const QStyleOptionGraphicsItem* option, QWidget*) > +{ > + page()->mainFrame()->render(painter, option->exposedRect.toRect()); > +} I think the paint() function should get a /*! \reimp */ comment, to make it clear in the API docs that this is just a re-implementation. > +/*! > + Makes \a page the new web page of the web view. > + > + The parent QObject of the provided page remains the owner > + of the object. If the current document is a child of the web > + view, it will be deleted. > + > + \sa page() The docs here say "web view" but should say "web graphicsitem". > +void QWebGraphicsItem::setUrl(const QUrl &url) > +{ > + if (url == page()->mainFrame()->url()) I think it's better to avoid the call to page() here and instead simply write: if (d->page && d->page->mainFrame()->url() == url) > +/*! > + \property QWebGraphicsItem::textSizeMultiplier > + \brief the scaling factor for all text in the frame > + \obsolete > + > + Use setZoomFactor instead, in combination with the > + ZoomTextOnly attribute in QWebSettings. > + > + \note Setting this property also enables the > + ZoomTextOnly attribute in QWebSettings. > + > + By default, this property contains a value of 1.0. > +*/ Please don't add this property. It's marked with \obsolete, there's no reason to add new stuff that is already obsolete :-) > +void QWebGraphicsItem::updateGeometry() > +{ > + QGraphicsWidget::updateGeometry(); > + QSize size = geometry().size().toSize(); > + page()->setViewportSize(size); > +} > + > +void QWebGraphicsItem::setGeometry(const QRectF& rect) > +{ > + QGraphicsWidget::setGeometry(rect); > + // NOTE: call geometry() as setGeometry ensures that > + // the geometry is within legal bounds (minimumSize, maximumSize) > + QSize size = geometry().size().toSize(); > + page()->setViewportSize(size); > +} If those two functions are re-implementations from QGraphicsLayoutItem, then they should be marked in the docs with /*! \reimp */ > +QString QWebGraphicsItem::status() const > +{ > + return d->statusBarMessage; > +} This function is missing documentation. > +/*! > + Sets the content of the web view to the specified \a html. > + > + External objects such as stylesheets or images referenced in the HTML > + document are located relative to \a baseUrl. > + > + The \a html is loaded immediately; external objects are loaded asynchronously. > + > + When using this method, WebKit assumes that external resources such as > + JavaScript programs or style sheets are encoded in UTF-8 unless otherwise > + specified. For example, the encoding of an external script can be specified > + through the charset attribute of the HTML script tag. Alternatively, the > + encoding can also be specified by the web server. > + > + \sa load(), setContent(), QWebFrame::toHtml() > +*/ The docs here mentioned "web view" again, where it should be "web graphicsitem". I see this is also the case in some of the other functions. > +void QWebGraphicsItem::hoverMoveEvent(QGraphicsSceneHoverEvent* ev) > +{ > + if (d->interactive && d->page) { > + const bool accepted = ev->isAccepted(); > + QMouseEvent me = QMouseEvent(QEvent::MouseMove, > + ev->pos().toPoint(), Qt::NoButton, > + Qt::NoButton, Qt::NoModifier); > + d->page->setView(ev->widget()); > + d->page->event(&me); > + ev->setAccepted(accepted); > + } > + > + if (!ev->isAccepted()) > + QGraphicsItem::hoverMoveEvent(ev); > +} > + > +void QWebGraphicsItem::hoverLeaveEvent(QGraphicsSceneHoverEvent* ev) > +{ > + Q_UNUSED(ev); > +} These two re-implementations are missing the \reimp tag in the docs. > +void QWebGraphicsItem::keyPressEvent(QKeyEvent* ev) Ditto :-) > + virtual void setGeometry(const QRectF& rect); > + virtual void updateGeometry(); > + void paint(QPainter*, const QStyleOptionGraphicsItem* options, QWidget* widget = 0); I like the use of the virtual keyword in header files for re-implemented functions, to indicate that it's a re-implementation. I think you forgot it for paint() though :-) > + QGraphicsScene* scene() { return m_scene; } > + QWebGraphicsItem* webItem() { return m_item; } const missing? :) It is a bit unfortunate that the QGVLauncher duplicates code from QtLauncher. It might be a good thing to clean up in the future. > diff --git a/WebKit/qt/WebCoreSupport/ChromeClientQt.cpp b/WebKit/qt/WebCoreSupport/ChromeClientQt.cpp > index d659833..e79ae55 100644 > --- a/WebKit/qt/WebCoreSupport/ChromeClientQt.cpp > +++ b/WebKit/qt/WebCoreSupport/ChromeClientQt.cpp > @@ -45,6 +45,7 @@ > #include "qwebframe_p.h" > #include "qwebsecurityorigin.h" > #include "qwebsecurityorigin_p.h" > +#include "qwebview.h" > > #include <qtooltip.h> > #include <qtextdocument.h> > @@ -307,8 +308,8 @@ void ChromeClientQt::repaint(const IntRect& windowRect, bool contentChanged, boo > { > // No double buffer, so only update the QWidget if content changed. > if (contentChanged) { > - QWidget* view = m_webPage->view(); > - if (view) { > + // Only do implicit paints for QWebView's > + if (QWebView* view = qobject_cast<QWebView*>(m_webPage->view())) { > QRect rect(windowRect); > rect = rect.intersected(QRect(QPoint(0, 0), m_webPage->viewportSize())); > if (!rect.isEmpty()) > @@ -323,8 +324,8 @@ void ChromeClientQt::repaint(const IntRect& windowRect, bool contentChanged, boo > > void ChromeClientQt::scroll(const IntSize& delta, const IntRect& scrollViewRect, const IntRect&) > { > - QWidget* view = m_webPage->view(); > - if (view) > + // Only do implicit paints for QWebView's > + if (QWebView* view = qobject_cast<QWebView*>(m_webPage->view())) > view->scroll(delta.width(), delta.height(), scrollViewRect); > emit m_webPage->scrollRequested(delta.width(), delta.height(), scrollViewRect); > } Hmm, I'm not so sure about these two changes. The WebGraphicsItem doesn't call setView(), right? So why is this needed? Oh, I just also forgot one more thing: The QWebGraphicsItem destructor calls setView(0) - should it do that? :) 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.
Created attachment 39156 [details]
patch 1 - v0.2 - add QWebGraphicsItem to the API and QGVLauncher sample application
patch 1 - v0.2
simon, thanks for your comments. all addressed.
(... qouted ) > It is a bit unfortunate that the QGVLauncher duplicates code from QtLauncher. > It > might be a good thing to clean up in the future. simon, i agree although we based code in qtlauncher, it is already very cleaned up :) really ... we chould actually clean up qtlauncher code instead. > > diff --git a/WebKit/qt/WebCoreSupport/ChromeClientQt.cpp b/WebKit/qt/WebCoreSupport/ChromeClientQt.cpp > > index d659833..e79ae55 100644 > > --- a/WebKit/qt/WebCoreSupport/ChromeClientQt.cpp > > +++ b/WebKit/qt/WebCoreSupport/ChromeClientQt.cpp > > @@ -45,6 +45,7 @@ > > #include "qwebframe_p.h" > > #include "qwebsecurityorigin.h" > > #include "qwebsecurityorigin_p.h" > > +#include "qwebview.h" > > > > #include <qtooltip.h> > > #include <qtextdocument.h> > > @@ -307,8 +308,8 @@ void ChromeClientQt::repaint(const IntRect& windowRect, bool contentChanged, boo > > { > > // No double buffer, so only update the QWidget if content changed. > > if (contentChanged) { > > - QWidget* view = m_webPage->view(); > > - if (view) { > > + // Only do implicit paints for QWebView's > > + if (QWebView* view = qobject_cast<QWebView*>(m_webPage->view())) { > > QRect rect(windowRect); > > rect = rect.intersected(QRect(QPoint(0, 0), m_webPage->viewportSize())); > > if (!rect.isEmpty()) > > @@ -323,8 +324,8 @@ void ChromeClientQt::repaint(const IntRect& windowRect, bool contentChanged, boo > > > > void ChromeClientQt::scroll(const IntSize& delta, const IntRect& scrollViewRect, const IntRect&) > > { > > - QWidget* view = m_webPage->view(); > > - if (view) > > + // Only do implicit paints for QWebView's > > + if (QWebView* view = qobject_cast<QWebView*>(m_webPage->view())) > > view->scroll(delta.width(), delta.height(), scrollViewRect); > > emit m_webPage->scrollRequested(delta.width(), delta.height(), scrollViewRect); > > } > > Hmm, I'm not so sure about these two changes. The WebGraphicsItem doesn't call > setView(), right? So why is this needed? 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. > Oh, I just also forgot one more thing: The QWebGraphicsItem destructor calls > setView(0) - should it do that? :) simon, QWebView's destructor does the same. Why should not we do the same here ? 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.
Created attachment 39165 [details]
patch 1 - v0.2.1 - add QWebGraphicsItem to the API and QGVLauncher sample application
same as "patch 1 - v0.2", but updated to tot.
no touch in "patch 2"
fwiw, this is my tree w/ latest patches http://gitorious.org/~tonikitoo/qtwebkit/tonikitoos-clone/commits/graphicsitem posted here 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.
for the reference, kenneth landed "patch 2" fixed and reviewed by Simon in r48177 Comment on attachment 39125 [details] patch 2 - v0.1 - Add support for handling QGraphicsScene events. marking "patch 2 - v0.1" as obsolete, based on action described in comment #30 Created attachment 39260 [details] (landed in r48219) patch 1 - v0.3 - add QWebGraphicsItem to the API and QGVLauncher sample application ( ariya's comments addressed. > 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 Comment on attachment 39260 [details] (landed in r48219) patch 1 - v0.3 - add QWebGraphicsItem to the API and QGVLauncher sample application ( r=me, this is a good start! More comments below of stuff that should be fixed in follow-up patches: > +protected: > + virtual void mousePressEvent(QGraphicsSceneMouseEvent*); > + virtual void mouseDoubleClickEvent(QGraphicsSceneMouseEvent*); > + virtual void mouseReleaseEvent(QGraphicsSceneMouseEvent*); > + virtual void mouseMoveEvent(QGraphicsSceneMouseEvent*); > + virtual void hoverMoveEvent(QGraphicsSceneHoverEvent*); > + virtual void hoverLeaveEvent(QGraphicsSceneHoverEvent*); > +#ifndef QT_NO_WHEELEVENT > + virtual void wheelEvent(QGraphicsSceneWheelEvent*); > +#endif > + virtual void keyPressEvent(QKeyEvent*); > + virtual void keyReleaseEvent(QKeyEvent*); > +#ifndef QT_NO_CONTEXTMENU > + virtual void contextMenuEvent(QContextMenuEvent*); > +#endif > + virtual void dragEnterEvent(QGraphicsSceneDragDropEvent*); > + virtual void dragLeaveEvent(QGraphicsSceneDragDropEvent*); > + virtual void dragMoveEvent(QGraphicsSceneDragDropEvent*); > + virtual void dropEvent(QGraphicsSceneDragDropEvent*); > + virtual void focusInEvent(QFocusEvent*); > + virtual void focusOutEvent(QFocusEvent*); > + virtual void inputMethodEvent(QInputMethodEvent*); > + virtual bool focusNextPrevChild(bool next); 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. > +++ b/WebKit/qt/QGVLauncher/main.cpp This file has a fewcoding style violations in them (* placement for example). Please fix before landing. > @@ -307,8 +308,8 @@ void ChromeClientQt::repaint(const IntRect& windowRect, bool contentChanged, boo > { > // No double buffer, so only update the QWidget if content changed. > if (contentChanged) { > - QWidget* view = m_webPage->view(); > - if (view) { > + // Only do implicit paints for QWebView's > + if (QWebView* view = qobject_cast<QWebView*>(m_webPage->view())) { > QRect rect(windowRect); > rect = rect.intersected(QRect(QPoint(0, 0), m_webPage->viewportSize())); > if (!rect.isEmpty()) > @@ -323,8 +324,8 @@ void ChromeClientQt::repaint(const IntRect& windowRect, bool contentChanged, boo > > void ChromeClientQt::scroll(const IntSize& delta, const IntRect& scrollViewRect, const IntRect&) > { > - QWidget* view = m_webPage->view(); > - if (view) > + // Only do implicit paints for QWebView's > + if (QWebView* view = qobject_cast<QWebView*>(m_webPage->view())) > view->scroll(delta.width(), delta.height(), scrollViewRect); > emit m_webPage->scrollRequested(delta.width(), delta.height(), scrollViewRect); > } 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. I hoi 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. Tor Arne, what do you feel about:
class QWebPageClient {
public:
virtual update(...) const = 0;
virtual scroll(...) const = 0;
virtual setWebCursor(const QCursor& cursor) { m_webCursor = cursor; }
QCursor lastWebCursor() const { return m_webCursor; }
virtual QCursor cursor() = 0;
virtual int screenNumber() = 0;
...
}
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. Comment on attachment 39260 [details] (landed in r48219) patch 1 - v0.3 - add QWebGraphicsItem to the API and QGVLauncher sample application ( Landed in 48219 I'd be tempted to name this class QWebGraphicsWidget instead, but it looks good to me. And unit tests! +/*!
+ \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. :)
Created attachment 39299 [details]
Followup: Implement some virtual event methods so that we can fix event-related bugs in Qt patch releases
> Very nice, Kenneth. :)
And Antonio of course. :) I apologise, should have read the bug carefully :(.
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 ..." Comment on attachment 39299 [details]
Followup: Implement some virtual event methods so that we can fix event-related bugs in Qt patch releases
Landed in 48221
> I think you can drop the comment, or at least fix the "allows" spelling and say
> something like "Re-implemented in order ..."
Missed this comment, fixed now.
Created attachment 39302 [details] Call the right base class function QGraphicsWidget::event() instead of skipping it and using QObject::event() instead. Patch by Simon Hausmann <hausmann@webkit.org> on 2009-09-09 Reviewed by NOBODY (OOPS!). * Api/qwebgraphicsitem.cpp: (QWebGraphicsItem::event): --- 2 files changed, 11 insertions(+), 1 deletions(-) Comment on attachment 39302 [details] Call the right base class function QGraphicsWidget::event() instead > - return QObject::event(event); > + return QGraphicsWidget::event(event); Oops, missed that one. Sorry :/ (In reply to comment #48) > (From update of attachment 39302 [details]) > > - return QObject::event(event); > > + return QGraphicsWidget::event(event); > > Oops, missed that one. Sorry :/ Auch! Thanks for fixing Simon. Comment on attachment 39302 [details] Call the right base class function QGraphicsWidget::event() instead Clearing review as it's been landed in r48246 I'm starting to think we should close this bug and continue with new bugs on follow-up issues, including QWebPageClient.
> 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. some follow up bugs: * Bug 29155 - [Qt] Implement autotests for QWebGraphicsItem * Bug 29156 - [Qt] Fix scrolling implementation on QWebGraphicsItem (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. |