WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
Bug 65920
[Qt] [WK2] Provide API for intercept (and possible ignore) links clicked in QDesktopWebView
https://bugs.webkit.org/show_bug.cgi?id=65920
Summary
[Qt] [WK2] Provide API for intercept (and possible ignore) links clicked in Q...
Caio Marcelo de Oliveira Filho
Reported
2011-08-09 07:14:30 PDT
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
.
Attachments
Patch
(20.31 KB, patch)
2011-08-09 11:34 PDT
,
Caio Marcelo de Oliveira Filho
no flags
Details
Formatted Diff
Diff
Patch
(20.85 KB, patch)
2011-08-18 15:05 PDT
,
Caio Marcelo de Oliveira Filho
benjamin
: review+
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Caio Marcelo de Oliveira Filho
Comment 1
2011-08-09 11:34:34 PDT
Created
attachment 103377
[details]
Patch
Benjamin Poulain
Comment 2
2011-08-10 10:56:23 PDT
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 3
2011-08-10 10:57:19 PDT
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
Comment 4
2011-08-10 11:38:23 PDT
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 5
2011-08-10 11:44:04 PDT
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
Comment 6
2011-08-11 08:02:55 PDT
> > > 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
Comment 7
2011-08-18 15:05:11 PDT
Created
attachment 104404
[details]
Patch
Caio Marcelo de Oliveira Filho
Comment 8
2011-08-18 15:35:12 PDT
(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 9
2011-08-19 09:26:00 PDT
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
Comment 10
2011-08-19 12:16:14 PDT
Committed
r93427
: <
http://trac.webkit.org/changeset/93427
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug