Bug 73818 - [Qt] Extend QQuickWebview::navigationRequested API
Summary: [Qt] Extend QQuickWebview::navigationRequested API
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Rafael Brandao
URL:
Keywords:
Depends on:
Blocks: 74403
  Show dependency treegraph
 
Reported: 2011-12-05 03:54 PST by Simon Hausmann
Modified: 2011-12-20 07:45 PST (History)
9 users (show)

See Also:


Attachments
Patch (11.83 KB, patch)
2011-12-14 10:04 PST, Rafael Brandao
no flags Details | Formatted Diff | Diff
Patch (15.12 KB, patch)
2011-12-15 12:10 PST, Rafael Brandao
hausmann: review+
hausmann: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Hausmann 2011-12-05 03:54:25 PST
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).
Comment 1 Rafael Brandao 2011-12-13 16:19:18 PST
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.
Comment 2 Rafael Brandao 2011-12-14 10:04:48 PST
Created attachment 119239 [details]
Patch
Comment 3 Alexis Menard (darktears) 2011-12-14 10:22:18 PST
Comment on attachment 119239 [details]
Patch

Is it possible to have some test?
Comment 4 Rafael Brandao 2011-12-14 10:52:30 PST
(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?
Comment 5 Rafael Brandao 2011-12-15 09:09:07 PST
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.
Comment 6 Rafael Brandao 2011-12-15 12:10:18 PST
Created attachment 119478 [details]
Patch
Comment 7 Simon Hausmann 2011-12-19 05:59:52 PST
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));
Comment 8 Simon Hausmann 2011-12-20 06:46:06 PST
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 :)
Comment 9 Simon Hausmann 2011-12-20 07:36:20 PST
Committed r103330: <http://trac.webkit.org/changeset/103330>
Comment 10 Csaba Osztrogonác 2011-12-20 07:40:09 PST
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
Comment 11 Simon Hausmann 2011-12-20 07:45:10 PST
(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