Bug 65920 – [Qt] [WK2] Provide API for intercept (and possible ignore) links clicked in QDesktopWebView

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Caio Marcelo de Oliveira Filho]

See bug 65919 for the whole picture. As a first step, implement the necessary plumbing and add a concrete (and QML-friendly) API in QDesktopWebView for enabling the "Open in new Tab" use case described in bug 65919.

[Caio Marcelo de Oliveira Filho]

Created attachment 103377 [details]
Patch

[Benjamin Poulain]

Comment on attachment 103377 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=103377&action=review

> Source/WebKit2/Shared/qt/WebCoreArgumentCodersQt.cpp:41
>  void ArgumentCoder<ResourceRequest>::encode(ArgumentEncoder* encoder, const ResourceRequest& resourceRequest)
>  {
> -    notImplemented();
> +    encoder->encode(resourceRequest.url().string());
>  }

Do you know why this is not done in common code? I wonder why encoding ResourceRequest is port specific.

> Source/WebKit2/Shared/qt/WebCoreArgumentCodersQt.cpp:45
> -    // FIXME: Add real implementation when we want to implement something that
> +    // FIXME: Add *more* coding implementation when we want to implement something that

Hehe

> Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:385
> +    if (QMetaObject::invokeMethod(q, "acceptLinkClicked", Q_RETURN_ARG(QVariant, ret), Q_ARG(QVariant, url), Q_ARG(QVariant, button), Q_ARG(QVariant, QVariant(modifiers))))
> +        return ret.toBool();
> +    return q->acceptLinkClicked(url, button, modifiers);

Do we need the virtual method at all?
Shouldn't that be?:
if (QMetaObject::invokeMethod(q, "acceptLinkClicked", Q_RETURN_ARG(QVariant, ret), Q_ARG(QVariant, url), Q_ARG(QVariant, button), Q_ARG(QVariant, QVariant(modifiers))))
    return ret.toBool();
return true;

> Source/WebKit2/UIProcess/API/qt/qdesktopwebview_p.h:30
> +class QDesktopWebViewPrivate : public WebKit::ViewInterface, public WebKit::PolicyInterface

I am not sure I would do that.
I guess you created PolicyInterface because you have more methods to add.

If possible, that could be a separate class handling its own business, like the context menu proxy.

> Source/WebKit2/UIProcess/qt/PolicyInterface.h:34
> +    enum PolicyAction {
> +        ActionUse,
> +        ActionDownload,
> +        ActionIgnore
> +    };

Defined but not use?

[Benjamin Poulain]

Comment on attachment 103377 [details]
Patch

The general idea looks good but I assume you have other function to add to the PolicyInterface.

R-ing to be evil.

[Caio Marcelo de Oliveira Filho]

Thanks for the review.


(In reply to comment #2)
> (From update of attachment 103377 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=103377&action=review
> 
> > Source/WebKit2/Shared/qt/WebCoreArgumentCodersQt.cpp:41
> >  void ArgumentCoder<ResourceRequest>::encode(ArgumentEncoder* encoder, const ResourceRequest& resourceRequest)
> >  {
> > -    notImplemented();
> > +    encoder->encode(resourceRequest.url().string());
> >  }
> 
> Do you know why this is not done in common code? I wonder why encoding ResourceRequest is port specific.

ResourceRequest itself is port specific, so the handling code follows the trend.

> > Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:385
> > +    if (QMetaObject::invokeMethod(q, "acceptLinkClicked", Q_RETURN_ARG(QVariant, ret), Q_ARG(QVariant, url), Q_ARG(QVariant, button), Q_ARG(QVariant, QVariant(modifiers))))
> > +        return ret.toBool();
> > +    return q->acceptLinkClicked(url, button, modifiers);
> 
> Do we need the virtual method at all?

The point was to provide API for C++ level API. I could just say that C++ would have to implement a SLOT with a certain name in the docs, but having it documented in the header file seemed more Qt-ish C++-ish.

It's not clear to me yet whether we consider the C++ API part of our "public commited offering" or just the QML items...

 
> > Source/WebKit2/UIProcess/API/qt/qdesktopwebview_p.h:30
> > +class QDesktopWebViewPrivate : public WebKit::ViewInterface, public WebKit::PolicyInterface
> 
> I am not sure I would do that.
> I guess you created PolicyInterface because you have more methods to add.

Actually right now I don't. I had to decide whether to put the method on the ViewInterface or somewhere else. Sounded wrong to put in ViewInterface, that's where PolicyInterface came up. Do you have another suggestion?


> If possible, that could be a separate class handling its own business, like the context menu proxy.

You mean for the PolicyInterface implementation? Or a separate proxy class instead of hooking things on QtWebPageProxy?

For the first it is WebView specific, since each WebView decide different things for handling policy (or ignoring).


> > Source/WebKit2/UIProcess/qt/PolicyInterface.h:34
> > +    enum PolicyAction {
> > +        ActionUse,
> > +        ActionDownload,
> > +        ActionIgnore
> > +    };
> 
> Defined but not use?

Garbage from a previous iteration, will be removed.

[Kenneth Rohde Christiansen]

Comment on attachment 103377 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=103377&action=review

> Source/WebKit2/UIProcess/API/qt/qdesktopwebview.h:104
> +    virtual bool acceptLinkClicked(const QUrl&, Qt::MouseButton, Qt::KeyboardModifiers);

Can't we find a better name? it sounds like accept-link clicked. and not like accept the link that was clicked. I dislike the "clicked" in the name. What if it was "touched"?

acceptNavigatingToUrl ? acceptLoadingUrl ? acceptUrlNavigationRequest ?

> Source/WebKit2/UIProcess/qt/PolicyInterface.h:33
> +        ActionUse,
> +        ActionDownload,
> +        ActionIgnore

Wouldn't it be more Qt'ish by appending Action? or just using "Use" "Download"... etc

[Benjamin Poulain]

> > > Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:385
> > > +    if (QMetaObject::invokeMethod(q, "acceptLinkClicked", Q_RETURN_ARG(QVariant, ret), Q_ARG(QVariant, url), Q_ARG(QVariant, button), Q_ARG(QVariant, QVariant(modifiers))))
> > > +        return ret.toBool();
> > > +    return q->acceptLinkClicked(url, button, modifiers);
> > 
> > Do we need the virtual method at all?
> 
> The point was to provide API for C++ level API. I could just say that C++ would have to implement a SLOT with a certain name in the docs, but having it documented in the header file seemed more Qt-ish C++-ish.
> 
> It's not clear to me yet whether we consider the C++ API part of our "public commited offering" or just the QML items...

Currently, the plan is to have no C++ API part in the public offering.

> > > Source/WebKit2/UIProcess/API/qt/qdesktopwebview_p.h:30
> > > +class QDesktopWebViewPrivate : public WebKit::ViewInterface, public WebKit::PolicyInterface
> > 
> > I am not sure I would do that.
> > I guess you created PolicyInterface because you have more methods to add.
> 
> Actually right now I don't. I had to decide whether to put the method on the ViewInterface or somewhere else. Sounded wrong to put in ViewInterface, that's where PolicyInterface came up. Do you have another suggestion?
>
> > If possible, that could be a separate class handling its own business, like the context menu proxy.
> 
> You mean for the PolicyInterface implementation? Or a separate proxy class instead of hooking things on QtWebPageProxy?
> 
> For the first it is WebView specific, since each WebView decide different things for handling policy (or ignoring).

The current design is fine. I was assuming the PolicyInterface would become some sort of a big controller with follow up patches. That is why I complained about the multiple inheritance.

[Caio Marcelo de Oliveira Filho]

Created attachment 104404 [details]
Patch

[Caio Marcelo de Oliveira Filho]

(In reply to comment #7)
> Created an attachment (id=104404) [details]
> Patch

This patch doesn't "protect" the callback from loads that came from the API, is a bit tricky to implement that. I'll still investigate, but I think this can be done in a separate patch.

I didn't see much value in passing the NavigationType parameter for now. I think the function already is a bit complicated, I'm not sure about the value the navigation type would bring to the table.

[Benjamin Poulain]

Comment on attachment 104404 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=104404&action=review

I like this patch a lot, I think that will be really useful. Nice work.

> Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:396
> +    if (QMetaObject::invokeMethod(q, "navigationPolicyForUrl", Q_RETURN_ARG(QVariant, ret), Q_ARG(QVariant, url), Q_ARG(QVariant, button), Q_ARG(QVariant, QVariant(modifiers))))

It would be nice to have the doc for that, otherwise it is not advertised anywhere that you can implement this function in QML.

> Source/WebKit2/UIProcess/API/qt/tests/qmltests/DesktopWebView/tst_navigationPolicyForUrl.qml:49
> +            mouseClick(webView, 100, 100, Qt.MiddleButton, Qt.ControlModifier)
> +            otherSpy.wait()
> +            compare(spy.count, 0)
> +            compare(otherSpy.count, 1)
> +            compare(otherWebView.title, "Test page 1")

This is one case, I think checking the UsePolicy would be necessary as well.
And you could check that the the original webview did not load anything since IgnorePolicy was returned.

[Caio Marcelo de Oliveira Filho]

Committed r93427: <http://trac.webkit.org/changeset/93427>