Bug 37858

Created attachment 53817 [details] Patch

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

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

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

Created attachment 53825 [details] Patch

Comment on attachment 53825 [details] Patch Rejecting patch 53825 from commit-queue. jesus@webkit.org 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 eseidel@chromium.org to request a commit-queue restart. After restart the commit-queue will correctly respect your committer rights.

I restarted the bot. It should recognize jesus now.

Comment on attachment 53825 [details] Patch Clearing flags on attachment: 53825 Committed r58038: <http://trac.webkit.org/changeset/58038>

All reviewed patches have been landed. Closing bug.

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.

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

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

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

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

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

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