Bug 37866 – [Qt] PageClientQt specific implementation for QGraphicsWidget

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Jesus Sanchez-Palencia]

[Qt] PageClientQt specific implementation for QGraphicsWidget

[Jesus Sanchez-Palencia]

Created attachment 53837 [details]
Patch

[Jesus Sanchez-Palencia]

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.

[Jesus Sanchez-Palencia]

Created attachment 53884 [details]
Patch

[Kenneth Rohde Christiansen]

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?

[Jesus Sanchez-Palencia]

(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!

[Kenneth Rohde Christiansen]

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.

[Jesus Sanchez-Palencia]

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).

[Eric Seidel (no email)]

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.

[Jesus Sanchez-Palencia]

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.

[WebKit Commit Bot]

Comment on attachment 55575 [details]
PageClientQGraphicsWidget

Clearing flags on attachment: 55575

Committed r59152: <http://trac.webkit.org/changeset/59152>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.