Bug 73818 – [Qt] Extend QQuickWebview::navigationRequested API

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[Simon Hausmann]

The NavigationRequest object provided as argument to QQuickWebView::navigationRequested provides the following properties:

    QUrl url (read-only)
    int button (read-only)
    int modifiers (read-only)
    int action (to be set)

On the WK2 C API level we get additional parameters:

    The frame the request belongs to
    The page the frame belongs to
    The navigation type

We should consider extending our API to at least expose the url of the frame requesting. This and perhaps other properties
may be essential in the decision making of the application about whether to accept or ignore the request.


While we're at it: The QQuickWebView::NavigationRequestAction enum (correspond go the action property) contains Accept, Ignore
as well as Download as values. Given that we do not intend to expose the download API initially, perhaps we should hide "Download"
from the public API (and make it only visible in experimental perhaps, i.e. you write request.action = WebView.experimental.DownloadRequest).

[Rafael Brandao]

Working on this, I've realized we get the WebProcess crashed once we try to accept a navigation request with Download (and this without my working patch). I will show this to Jesus tomorrow to see if this actually a bug or if I'm doing something wrong.

Besides the frame requesting url, would we need anything else? So I could also add it when I submit it tomorrow.

[Rafael Brandao]

Created attachment 119239 [details]
Patch

[Alexis Menard (darktears)]

Comment on attachment 119239 [details]
Patch

Is it possible to have some test?

[Rafael Brandao]

(In reply to comment #3)
> (From update of attachment 119239 [details])
> Is it possible to have some test?

Sure. I didn't put this right away because it was not working before my patch, I confirmed with Jesus today that it is a bug. As there was no qml test for it before, we just missed that regression.

Do you think I should add a test that fails here so it could be fixed in another patch?

[Rafael Brandao]

The test is gonna be added in a separate patch: https://bugs.webkit.org/show_bug.cgi?id=74541

When it lands, I will just change it here in this one to reflect the change in the enum.

[Rafael Brandao]

Created attachment 119478 [details]
Patch

[Simon Hausmann]

Comment on attachment 119478 [details]
Patch

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

Looks good in general, but there's a small memory leak that should be fixed.

> Source/WebKit2/UIProcess/qt/QtWebPagePolicyClient.cpp:105
> +    WKURLRef frameURL = WKFrameCopyURL(frame);
>      WKURLRef requestURL = WKURLRequestCopyURL(request);

Memory leak, the url refs need to be kept in RetainPtrs and adopted:

WKRetainPtr<WKURLRef> frameURL(AdoptWK, WKFrameCopyURL(frame));

[Simon Hausmann]

Comment on attachment 119478 [details]
Patch

I'd like to get this change in, so r=me and I'll fix the leak when landing (trivial :)

[Simon Hausmann]

Committed r103330: <http://trac.webkit.org/changeset/103330>

[Csaba Osztrogonác]

It broke the build:

cc1plus: warnings being treated as errors
/ramdisk/qt-linux-32-release-webkit2/build/Source/WebKit2/UIProcess/qt/QtWebPagePolicyClient.cpp: In member function ‘void QtWebPagePolicyClient::decidePolicyForNavigationAction(const QUrl&, const QUrl&, Qt::MouseButton, Qt::KeyboardModifiers, const OpaqueWKFramePolicyListener*)’:
/ramdisk/qt-linux-32-release-webkit2/build/Source/WebKit2/UIProcess/qt/QtWebPagePolicyClient.cpp:56: error: case label value exceeds maximum value for type

[Simon Hausmann]

(In reply to comment #10)
> It broke the build:
> 
> cc1plus: warnings being treated as errors
> /ramdisk/qt-linux-32-release-webkit2/build/Source/WebKit2/UIProcess/qt/QtWebPagePolicyClient.cpp: In member function ‘void QtWebPagePolicyClient::decidePolicyForNavigationAction(const QUrl&, const QUrl&, Qt::MouseButton, Qt::KeyboardModifiers, const OpaqueWKFramePolicyListener*)’:
> /ramdisk/qt-linux-32-release-webkit2/build/Source/WebKit2/UIProcess/qt/QtWebPagePolicyClient.cpp:56: error: case label value exceeds maximum value for type

Fixed with http://trac.webkit.org/changeset/103331