Bug 37858 - [Qt] PageClientQt specific implementation for QWidget
Summary: [Qt] PageClientQt specific implementation for QWidget
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Jesus Sanchez-Palencia
URL:
Keywords: Qt
Depends on:
Blocks: 37856 37866
  Show dependency treegraph
 
Reported: 2010-04-20 07:16 PDT by Jesus Sanchez-Palencia
Modified: 2010-05-07 08:02 PDT (History)
6 users (show)

See Also:


Attachments
Patch (11.29 KB, patch)
2010-04-20 07:44 PDT, Jesus Sanchez-Palencia
no flags Details | Formatted Diff | Diff
Patch (11.35 KB, patch)
2010-04-20 08:33 PDT, Jesus Sanchez-Palencia
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jesus Sanchez-Palencia 2010-04-20 07:16:34 PDT
[Qt] PageClientQt specific implementation for QWidget
Comment 1 Jesus Sanchez-Palencia 2010-04-20 07:44:07 PDT
Created attachment 53817 [details]
Patch
Comment 2 Early Warning System Bot 2010-04-20 07:52:04 PDT
Attachment 53817 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/1728111
Comment 3 Antonio Gomes 2010-04-20 08:09:59 PDT
(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?
Comment 4 Jesus Sanchez-Palencia 2010-04-20 08:22:26 PDT
(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.
Comment 5 Jesus Sanchez-Palencia 2010-04-20 08:33:16 PDT
Created attachment 53825 [details]
Patch
Comment 6 WebKit Commit Bot 2010-04-20 10:30:12 PDT
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.
Comment 7 Eric Seidel (no email) 2010-04-21 04:15:23 PDT
I restarted the bot. It should recognize jesus now.
Comment 8 WebKit Commit Bot 2010-04-21 18:56:36 PDT
Comment on attachment 53825 [details]
Patch

Clearing flags on attachment: 53825

Committed r58038: <http://trac.webkit.org/changeset/58038>
Comment 9 WebKit Commit Bot 2010-04-21 18:56:42 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Simon Hausmann 2010-04-26 03:23:08 PDT
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.
Comment 11 Kenneth Rohde Christiansen 2010-04-26 05:25:02 PDT
(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.
Comment 12 Simon Hausmann 2010-04-26 07:06:28 PDT
(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.
Comment 13 Jesus Sanchez-Palencia 2010-04-26 11:11:32 PDT
> 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!
Comment 14 Kenneth Rohde Christiansen 2010-04-26 13:17:35 PDT
(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.
Comment 15 Kenneth Rohde Christiansen 2010-04-26 14:07:12 PDT
> 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.
Comment 16 Simon Hausmann 2010-05-07 08:02:52 PDT
Removing this from the blockers, as the refactoring doesn't really fix anything in the release branch :)