Bug 73145 - [Qt][WK2] Move startDrag implementation to QtWebPageEventHandler
Summary: [Qt][WK2] Move startDrag implementation to QtWebPageEventHandler
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jesus Sanchez-Palencia
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks: 72943
  Show dependency treegraph
 
Reported: 2011-11-25 14:40 PST by Jesus Sanchez-Palencia
Modified: 2011-12-12 06:55 PST (History)
5 users (show)

See Also:


Attachments
Patch (9.90 KB, patch)
2011-11-25 14:53 PST, Jesus Sanchez-Palencia
no flags Details | Formatted Diff | Diff
Patch (9.46 KB, patch)
2011-12-05 10:33 PST, Jesus Sanchez-Palencia
no flags Details | Formatted Diff | Diff
Patch (9.46 KB, patch)
2011-12-06 07:03 PST, Jesus Sanchez-Palencia
no flags Details | Formatted Diff | Diff
Patch (10.96 KB, patch)
2011-12-09 07:25 PST, Jesus Sanchez-Palencia
hausmann: review+
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 2011-11-25 14:40:26 PST
We can move QtWebPageProxy::startDrag impl to QtWebPageEventHandler.
Comment 1 Jesus Sanchez-Palencia 2011-11-25 14:53:47 PST
Created attachment 116641 [details]
Patch
Comment 2 Kenneth Rohde Christiansen 2011-11-25 15:51:48 PST
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...
Comment 3 Jesus Sanchez-Palencia 2011-11-28 04:31:04 PST
(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 4 Caio Marcelo de Oliveira Filho 2011-12-01 12:18:34 PST
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?
Comment 5 Jesus Sanchez-Palencia 2011-12-05 10:33:27 PST
Created attachment 117897 [details]
Patch
Comment 6 Simon Hausmann 2011-12-06 04:03:18 PST
(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)
Comment 7 Jesus Sanchez-Palencia 2011-12-06 06:41:22 PST
(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.
Comment 8 Jesus Sanchez-Palencia 2011-12-06 07:03:06 PST
Created attachment 118044 [details]
Patch
Comment 9 Jesus Sanchez-Palencia 2011-12-09 07:25:47 PST
Created attachment 118573 [details]
Patch
Comment 10 WebKit Review Bot 2011-12-12 06:26:13 PST
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 11 Jesus Sanchez-Palencia 2011-12-12 06:27:25 PST
Comment on attachment 118573 [details]
Patch

I will rebase the patch due to a conflict and land it manually. 
Thanks for the review, Simon! :)
Comment 12 Jesus Sanchez-Palencia 2011-12-12 06:55:17 PST
Committed r102579: <http://trac.webkit.org/changeset/102579>