Summary: | [Qt][WK2] Move startDrag implementation to QtWebPageEventHandler | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jesus Sanchez-Palencia <jesus> | ||||||||||
Component: | WebKit Qt | Assignee: | Jesus Sanchez-Palencia <jesus> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | cmarcelo, hausmann, kenneth, webkit.review.bot, zoltan | ||||||||||
Priority: | P2 | Keywords: | Qt, QtTriaged | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 72943 | ||||||||||||
Attachments: |
|
Description
Jesus Sanchez-Palencia
2011-11-25 14:40:26 PST
Created attachment 116641 [details]
Patch
Comment on attachment 116641 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=116641&action=review > Source/WebKit2/UIProcess/qt/QtWebPageEventHandler.cpp:84 > -QtWebPageEventHandler::QtWebPageEventHandler(WKPageRef pageRef, WebKit::QtViewportInteractionEngine* viewportInteractionEngine) > +QtWebPageEventHandler::QtWebPageEventHandler(WKPageRef pageRef, QQuickWebView* qmlWebView, WebKit::QtViewportInteractionEngine* viewportInteractionEngine) I am not sure this is really cleaner. With this the page event handler has a reference to the view. Shouldn't or couldn't this be delegates to the webPageProxy instead? > Source/WebKit2/UIProcess/qt/QtWebPageEventHandler.cpp:408 > + if (QWindow* window = m_qmlWebView->canvas()) { > + QDrag* drag = new QDrag(window); This really sucks... (In reply to comment #2) > I am not sure this is really cleaner. With this the page event handler has a reference to the view. Shouldn't or couldn't this be delegates to the webPageProxy instead? At a first glance I thought the same about the reference to the view, but then I realized that we might need it in the future and starting with this functions didn't seem to be wrong, as it is dragging/event related. > > > Source/WebKit2/UIProcess/qt/QtWebPageEventHandler.cpp:408 > > + if (QWindow* window = m_qmlWebView->canvas()) { > > + QDrag* drag = new QDrag(window); > > This really sucks... Well, yes, but it's Qt related. The implementation isn't mine and I haven't investigated another way of doing this. Comment on attachment 116641 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=116641&action=review > Source/WebKit2/UIProcess/qt/QtWebPageEventHandler.cpp:25 > +#include <qquickcanvas.h> What about "<QtDeclarative/QQuickCanvas>"? > Source/WebKit2/UIProcess/qt/QtWebPageEventHandler.cpp:26 > + Maybe remove this blank line? >> Source/WebKit2/UIProcess/qt/QtWebPageEventHandler.cpp:84 >> +QtWebPageEventHandler::QtWebPageEventHandler(WKPageRef pageRef, QQuickWebView* qmlWebView, WebKit::QtViewportInteractionEngine* viewportInteractionEngine) > > I am not sure this is really cleaner. With this the page event handler has a reference to the view. Shouldn't or couldn't this be delegates to the webPageProxy instead? I think it is fine for event handler to reference the webview in this case. The whole QtWebPageProxy trouble started when we thought of it as a gate for everything that goes from WebKit to our WebView and vice-versa. The recent patches moved away from this approach. We tried to use the existing split in WebKit2 (the recent splits based on the WK2 C API), and we are doing our own split when it makes sense, like in the EventHandler. I see that we could make a EventHandlerClient for EventHandler interact with, that would be implemented by one client (either WebView or someone on behalf of it) but I don't think this will bring us anything right now. We can grow the engineering here when we need it. > Source/WebKit2/UIProcess/qt/QtWebPageEventHandler.h:66 > + QQuickWebView* m_qmlWebView; What about renaming to m_webView? Created attachment 117897 [details]
Patch
(In reply to comment #4) > (From update of attachment 116641 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=116641&action=review > > > Source/WebKit2/UIProcess/qt/QtWebPageEventHandler.cpp:25 > > +#include <qquickcanvas.h> > > What about "<QtDeclarative/QQuickCanvas>"? The module name in #include <Module/Classname> is only necessary for public header files. It is completely redundant for cpp files or internal header files. I'd go for #include <QQuickCanvas>, but I think qquickcanvas.h is also fine. (We need it for public header files to avoid requiring QT += that_other_module_webkit_depends_on in the app .pro files) (In reply to comment #6) > (We need it for public header files to avoid requiring QT += that_other_module_webkit_depends_on in the app .pro files) Thanks for the explanation and review, Simon. I will stick to <QQuickCanvas> then. The updated and rebased patch is coming soon. Created attachment 118044 [details]
Patch
Created attachment 118573 [details]
Patch
Comment on attachment 118573 [details] Patch Rejecting attachment 118573 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: andler.cpp Hunk #6 FAILED at 80. Hunk #7 succeeded at 397 (offset 1 line). 1 out of 7 hunks FAILED -- saving rejects to file Source/WebKit2/UIProcess/qt/QtWebPageEventHandler.cpp.rej patching file Source/WebKit2/UIProcess/qt/QtWebPageEventHandler.h patching file Source/WebKit2/UIProcess/qt/QtWebPageProxy.cpp patching file Source/WebKit2/UIProcess/qt/QtWebPageProxy.h Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Simon Hausmann', u'--f..." exit_code: 1 Full output: http://queues.webkit.org/results/10842098 Comment on attachment 118573 [details]
Patch
I will rebase the patch due to a conflict and land it manually.
Thanks for the review, Simon! :)
Committed r102579: <http://trac.webkit.org/changeset/102579> |