Bug 37858 – [Qt] PageClientQt specific implementation for QWidget

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jesus Sanchez-Palencia]

[Qt] PageClientQt specific implementation for QWidget

[Jesus Sanchez-Palencia]

Created attachment 53817 [details]
Patch

[Early Warning System Bot]

Attachment 53817 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/1728111

[Antonio Gomes]

(In reply to comment #1)
> Created an attachment (id=53817) [details]
> Patch

hi jeez!

could you please explain why it is QWiget/QWebView only, or if it is just a incremental part of the whole thing?

[Jesus Sanchez-Palencia]

(In reply to comment #3)
> could you please explain why it is QWiget/QWebView only, or if it is just a
> incremental part of the whole thing?

Yes! This is the first patch. The next one will bring QGraphicsWidget specific implementation.

[Jesus Sanchez-Palencia]

Created attachment 53825 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 53825 [details]
Patch

Rejecting patch 53825 from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in WebKitTools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  Due to bug 30084 the commit-queue will require a restart after your change.  Please contact [e-mail] to request a commit-queue restart.  After restart the commit-queue will correctly respect your committer rights.

[Eric Seidel (no email)]

I restarted the bot. It should recognize jesus now.

[WebKit Commit Bot]

Comment on attachment 53825 [details]
Patch

Clearing flags on attachment: 53825

Committed r58038: <http://trac.webkit.org/changeset/58038>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Hausmann]

The main reason for cherry-picking this change into the release branch is to simplify future cherry-picks?

We're a few weeks away from the release, and I'd like to get a better understanding about why we integrate certain changes.

[Kenneth Rohde Christiansen]

(In reply to comment #10)
> The main reason for cherry-picking this change into the release branch is to
> simplify future cherry-picks?
> 
> We're a few weeks away from the release, and I'd like to get a better
> understanding about why we integrate certain changes.

There is a follow up patch that fixes the KDE bugs, where it is impossible to implement a custom view and have plugins, cursors etc working correctly. Think Plasma.

[Simon Hausmann]

(In reply to comment #11)
> (In reply to comment #10)
> > The main reason for cherry-picking this change into the release branch is to
> > simplify future cherry-picks?
> > 
> > We're a few weeks away from the release, and I'd like to get a better
> > understanding about why we integrate certain changes.
> 
> There is a follow up patch that fixes the KDE bugs, where it is impossible to
> implement a custom view and have plugins, cursors etc working correctly. Think
> Plasma.

This is crash fix for a use-case that is currently not supported. It would be better if KDE used QGraphicsWebView for now.

This appears like an easy thing to fix. However if you take into account that for things like the tiled backing store and the accelerated compositing a deep integration into the item hierarchy is necessary (see rootlayer and overlay items), then this does not appear like a simple refactoring anymore.

QWebView use to be a thin layer on top of QWebPage, and so _was_ QGraphicsWebView. It isn't anymore and perhaps therefore we should reconsider if this is a valid use-case in the first place.

That said, the refactoring is certainly a nice code cleanup.

[Jesus Sanchez-Palencia]

> This is crash fix for a use-case that is currently not supported. It would be
> better if KDE used QGraphicsWebView for now.

Agreed, but since they didn't I used this as a chance for having the PageClient refactoring done.


> This appears like an easy thing to fix. However if you take into account that
> for things like the tiled backing store and the accelerated compositing a deep
> integration into the item hierarchy is necessary (see rootlayer and overlay
> items), then this does not appear like a simple refactoring anymore.

Yes, that is true! I have this other patch (https://bugs.webkit.org/show_bug.cgi?id=37866) that deal with the QGraphicsWebView side, but it's still pending for review. It would be awesome if you could review it, Simon.


> QWebView use to be a thin layer on top of QWebPage, and so _was_
> QGraphicsWebView. It isn't anymore and perhaps therefore we should reconsider
> if this is a valid use-case in the first place.
> 
> That said, the refactoring is certainly a nice code cleanup.

I see... Do you think that we shouldn't cherry-pick this for QtWebKit 2.0 and maybe leave it for QtWebKit 2.1? Or don't you want this at all for now?


Thanks for the comments!

[Kenneth Rohde Christiansen]

(In reply to comment #13)
> > This is crash fix for a use-case that is currently not supported. It would be
> > better if KDE used QGraphicsWebView for now.

Actually, with our API, it seems that we support people creating custom views around the QWebPage, but so far that has been a half-true and a pitfall that some of our users have fallen into. This patch makes it possible to implement a QGraphicsWidget based view using a QWebPage, just as we support implementing a QWidget based view using a QWebPage.

True, we might move away for this in the future, especially with the new WebKit2 API etc, but as KDE will not abandon their Plasme::WebView due to ABI/API compatibility this will help them.

[Kenneth Rohde Christiansen]

> This patch makes it possible to implement a
> QGraphicsWidget based view using a QWebPage, just as we support implementing a
> QWidget based view using a QWebPage.

I guess I confused this one with the follow up patch.

[Simon Hausmann]

Removing this from the blockers, as the refactoring doesn't really fix anything in the release branch :)