Bug 72976

Summary:

[Qt] [WK2] Expose onNavigationRequested signal instead of expecting a slot be defined from QML

Product:

WebKit

Reporter:

Caio Marcelo de Oliveira Filho <cmarcelo>

Component:

New Bugs

Assignee:

Caio Marcelo de Oliveira Filho <cmarcelo>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

cshu, hausmann, kbalazs, menard, vestbo, webkit.review.bot, zoltan

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

72959

Bug Blocks:

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	vestbo: review+

Caio Marcelo de Oliveira Filho

Reported 2011-11-22 11:30:26 PST

[Qt] [WK2] Expose onNavigationRequested signal instead of expecting a slot be defined from QML

Attachments
Patch (16.22 KB, patch) 2011-11-22 12:02 PST, Caio Marcelo de Oliveira Filho	no flags	Details Formatted Diff Diff
Patch (17.67 KB, patch) 2011-11-22 13:11 PST, Caio Marcelo de Oliveira Filho	no flags	Details Formatted Diff Diff
Patch (17.70 KB, patch) 2011-11-23 06:49 PST, Caio Marcelo de Oliveira Filho	vestbo: review+	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Caio Marcelo de Oliveira Filho

Comment 1 2011-11-22 11:57:18 PST

For some context, see the thread https://lists.webkit.org/pipermail/webkit-qt/2011-October/001984.html

Caio Marcelo de Oliveira Filho

Comment 2 2011-11-22 12:02:05 PST

Created attachment 116260 [details] Patch

Tor Arne Vestbø

Comment 3 2011-11-22 12:09:41 PST

Comment on attachment 116260 [details] Patch lgtm

Alexis Menard (darktears)

Comment 4 2011-11-22 12:21:32 PST

Comment on attachment 116260 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=116260&action=review > Source/WebKit2/UIProcess/qt/QtWebPagePolicyClient.cpp:57 > + void download() { m_action = WKFramePolicyListenerDownload; } I don't like the API. onNavigationRequested { request.download() } feels like it will actually do the download in a sense that I will download the request itself (a bit like file.open()). Maybe the name is just bad. Perhaps we should do like request.action = NavigationRequest.Download or request.action = NavigationRequest.Ignore or request.action = NavigationRequest.Use (the later should probably get another name).

Tor Arne Vestbø

Comment 5 2011-11-22 12:26:11 PST

(In reply to comment #4) > (From update of attachment 116260 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=116260&action=review > > > Source/WebKit2/UIProcess/qt/QtWebPagePolicyClient.cpp:57 > > + void download() { m_action = WKFramePolicyListenerDownload; } > > I don't like the API. onNavigationRequested { request.download() } feels like it will actually do the download in a sense that I will download the request itself (a bit like file.open()). Maybe the name is just bad. Perhaps we should do like request.action = NavigationRequest.Download or request.action = NavigationRequest.Ignore or request.action = NavigationRequest.Use (the later should probably get another name). We could name it triggerDownload(), to signal that it's not immediate? If we do enums I'd prefer if they were part of the WebView namespace, so action = WebView.AcceptRequest|WebView.IgnoreRequest|WebView.DownloadRequest

Alexis Menard (darktears)

Comment 6 2011-11-22 12:31:34 PST

(In reply to comment #5) > (In reply to comment #4) > > (From update of attachment 116260 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=116260&action=review > > > > > Source/WebKit2/UIProcess/qt/QtWebPagePolicyClient.cpp:57 > > > + void download() { m_action = WKFramePolicyListenerDownload; } > > > > I don't like the API. onNavigationRequested { request.download() } feels like it will actually do the download in a sense that I will download the request itself (a bit like file.open()). Maybe the name is just bad. Perhaps we should do like request.action = NavigationRequest.Download or request.action = NavigationRequest.Ignore or request.action = NavigationRequest.Use (the later should probably get another name). > > We could name it triggerDownload(), to signal that it's not immediate? If we do enums I'd prefer if they were part of the WebView namespace, so action = WebView.AcceptRequest|WebView.IgnoreRequest|WebView.DownloadRequest I prefer the second option much better :D

Caio Marcelo de Oliveira Filho

Comment 7 2011-11-22 13:11:58 PST

Created attachment 116268 [details] Patch

Caio Marcelo de Oliveira Filho

Comment 8 2011-11-22 13:12:54 PST

This patch depends on bug 72959, that's why is not applying.

Alexis Menard (darktears)

Comment 9 2011-11-22 13:16:22 PST

Comment on attachment 116268 [details] Patch I prefer like this :D

Rafael Brandao

Comment 10 2011-11-22 14:17:24 PST

Comment on attachment 116268 [details] Patch LGTM :)