Bug 37866

Summary: [Qt] PageClientQt specific implementation for QGraphicsWidget
Product: WebKit Reporter: Jesus Sanchez-Palencia <jesus>
Component: WebKit QtAssignee: Jesus Sanchez-Palencia <jesus>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, kenneth, laszlo.gombos
Priority: P2 Keywords: Qt
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 37858    
Bug Blocks: 35051, 37856    
Attachments:
Description Flags
Patch
none
Patch
none
PageClientQGraphicsWidget
none
PageClientQGraphicsWidget none

Description Jesus Sanchez-Palencia 2010-04-20 10:08:17 PDT
[Qt] PageClientQt specific implementation for QGraphicsWidget
Comment 1 Jesus Sanchez-Palencia 2010-04-20 10:20:16 PDT
Created attachment 53837 [details]
Patch
Comment 2 Jesus Sanchez-Palencia 2010-04-20 10:41:13 PDT
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.
Comment 3 Jesus Sanchez-Palencia 2010-04-20 15:00:00 PDT
Created attachment 53884 [details]
Patch
Comment 4 Kenneth Rohde Christiansen 2010-04-27 08:16:27 PDT
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?
Comment 5 Jesus Sanchez-Palencia 2010-04-27 09:40:46 PDT
(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!
Comment 6 Kenneth Rohde Christiansen 2010-04-29 05:23:19 PDT
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.
Comment 7 Jesus Sanchez-Palencia 2010-05-04 06:26:32 PDT
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 8 Eric Seidel (no email) 2010-05-08 23:09:49 PDT
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.
Comment 9 Jesus Sanchez-Palencia 2010-05-10 11:10:01 PDT
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 10 WebKit Commit Bot 2010-05-11 08:45:37 PDT
Comment on attachment 55575 [details]
PageClientQGraphicsWidget

Clearing flags on attachment: 55575

Committed r59152: <http://trac.webkit.org/changeset/59152>
Comment 11 WebKit Commit Bot 2010-05-11 08:45:45 PDT
All reviewed patches have been landed.  Closing bug.