Bug 73818

Summary: [Qt] Extend QQuickWebview::navigationRequested API
Product: WebKit Reporter: Simon Hausmann <hausmann>
Component: WebKit APIAssignee: Rafael Brandao <rafael.lobo>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarcelo, gopal.1.raghavan, jesus, laszlo.gombos, menard, ossy, vestbo, webkit.review.bot, zoltan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 74403    
Attachments:
Description Flags
Patch
none
Patch hausmann: review+, hausmann: commit-queue-

Simon Hausmann
Reported 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).
Attachments
Patch (11.83 KB, patch)
2011-12-14 10:04 PST, Rafael Brandao 		no flags
DetailsFormatted DiffDiff
Patch (15.12 KB, patch)
2011-12-15 12:10 PST, Rafael Brandao 		hausmann: review+
hausmann: commit-queue-
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Rafael Brandao
Comment 1 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.
Rafael Brandao
Comment 2 2011-12-14 10:04:48 PST
Created attachment 119239 [details] Patch
Alexis Menard (darktears)
Comment 3 2011-12-14 10:22:18 PST
Comment on attachment 119239 [details] Patch Is it possible to have some test?
Rafael Brandao
Comment 4 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?
Rafael Brandao
Comment 5 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.
Rafael Brandao
Comment 6 2011-12-15 12:10:18 PST
Created attachment 119478 [details] Patch
Simon Hausmann
Comment 7 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));
Simon Hausmann
Comment 8 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 :)
Simon Hausmann
Comment 9 2011-12-20 07:36:20 PST
Committed r103330: <http://trac.webkit.org/changeset/103330>
Csaba Osztrogonác
Comment 10 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
Simon Hausmann
Comment 11 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
Note You need to log in before you can comment on or make changes to this bug.