Bug 73145 – [Qt][WK2] Move startDrag implementation to QtWebPageEventHandler

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Jesus Sanchez-Palencia]

We can move QtWebPageProxy::startDrag impl to QtWebPageEventHandler.

[Jesus Sanchez-Palencia]

Created attachment 116641 [details]
Patch

[Kenneth Rohde Christiansen]

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

[Jesus Sanchez-Palencia]

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

[Caio Marcelo de Oliveira Filho]

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?

[Jesus Sanchez-Palencia]

Created attachment 117897 [details]
Patch

[Simon Hausmann]

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

[Jesus Sanchez-Palencia]

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

[Jesus Sanchez-Palencia]

Created attachment 118044 [details]
Patch

[Jesus Sanchez-Palencia]

Created attachment 118573 [details]
Patch

[WebKit Review Bot]

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

[Jesus Sanchez-Palencia]

Comment on attachment 118573 [details]
Patch

I will rebase the patch due to a conflict and land it manually. 
Thanks for the review, Simon! :)

[Jesus Sanchez-Palencia]

Committed r102579: <http://trac.webkit.org/changeset/102579>