Bug 37866

Created attachment 53837 [details] Patch

The proposed patch aims on solving the custom views issues that we have right now, when dealing with QGraphicsWidget based classes. In summary this patch: - removes the inheritance from QGraphicsWebViewPrivate, by separating the PageClientQGraphicsWidget into another class. - moves all needed functions, including tiling and AC ones, to the new PageClient implementation, making QGraphicsWebViewPrivate way cleaner. - adds QWebPage::setView(QGraphicsWidget*) making it possible to one to create a custom web view for any QGraphicsWidget.

Created attachment 53884 [details] Patch

Comment on attachment 53884 [details] Patch WebKit/qt/Api/qgraphicswebview.cpp:489 + d->overlay->prepareGraphicsItemGeometryChange(); You did this name change? WebKit/qt/Api/qgraphicswebview.cpp:853 + static_cast<PageClientQGraphicsWidget*>(d->page->d->client)->viewResizesToContents = enabled; I don't like having view specific things in the page client, if we can avoid it. Can we? WebKit/qt/ChangeLog:8 + including Tiling and Accelerated Composite specific ones. So now if KDE would use this, they will get tiling support?

(In reply to comment #4) > (From update of attachment 53884 [details]) > WebKit/qt/Api/qgraphicswebview.cpp:489 > + d->overlay->prepareGraphicsItemGeometryChange(); > You did this name change? Yes, since it was just an encapsulation for a protected function from QGraphicsItem. > > WebKit/qt/Api/qgraphicswebview.cpp:853 > + > static_cast<PageClientQGraphicsWidget*>(d->page->d->client)->viewResizesToContents > = enabled; > I don't like having view specific things in the page client, if we can avoid > it. Can we? This is just used on createOrDeleteOverlay(), which is now part of the PageClient (Tiling and AC need it). > > WebKit/qt/ChangeLog:8 > + including Tiling and Accelerated Composite specific ones. > So now if KDE would use this, they will get tiling support? Hmm, yes, they would have the support for it but they would need to refer to the QGraphicsWebView implementation and see what else is needed to enable it. It's just a matter of calling the proper functions of the PageClient and connecting the right signals to the right slots... ;) I don't know Tiling and AC implementation deeply, but from my tests I'd say that there are no regressions with this patch. No'am took a look at it and it seemed fine from the AC point of view. thanks for the review!

It generally looks good. It is a bit hard to review doe to all the code shuffeling, but I understand that it was not possible to split the patch up into parts. I'm OK with landing the patch, but if we find any regression we will have to roll it out. You have to keep that in mind. If everything looks fine after a week or so ,we can consider cherry-picking it.

Created attachment 55014 [details] PageClientQGraphicsWidget I've rebased the patch and added an autotest based on a test case from bug 35051 (https://bugs.webkit.org/attachment.cgi?id=50694).

Comment on attachment 53884 [details] Patch Cleared Simon Hausmann's review+ from obsolete attachment 53884 [details] so that this bug does not appear in http://webkit.org/pending-commit.

Created attachment 55575 [details] PageClientQGraphicsWidget Now this patch is just a code refactor, removing the inheritance from QGraphicsWebViewPrivate. The new setView and view() API for QWebPage will come in the future, on a new bug.

Comment on attachment 55575 [details] PageClientQGraphicsWidget Clearing flags on attachment: 55575 Committed r59152: <http://trac.webkit.org/changeset/59152>

All reviewed patches have been landed. Closing bug.