|Summary:||[Qt] Extend QQuickWebview::navigationRequested API|
|Product:||WebKit||Reporter:||Simon Hausmann <hausmann>|
|Component:||WebKit API||Assignee:||Rafael Brandao <rafael.lobo>|
|Severity:||Normal||CC:||cmarcelo, gopal.1.raghavan, jesus, laszlo.gombos, menard, ossy, vestbo, webkit.review.bot, zoltan|
|Version:||528+ (Nightly build)|
|Bug Depends on:|
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 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 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