Bug 72976 - [Qt] [WK2] Expose onNavigationRequested signal instead of expecting a slot be defined from QML
Summary: [Qt] [WK2] Expose onNavigationRequested signal instead of expecting a slot be...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Caio Marcelo de Oliveira Filho
URL:
Keywords:
Depends on: 72959
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-22 11:30 PST by Caio Marcelo de Oliveira Filho
Modified: 2011-11-23 08:56 PST (History)
7 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Caio Marcelo de Oliveira Filho 2011-11-22 11:30:26 PST
[Qt] [WK2] Expose onNavigationRequested signal instead of expecting a slot be defined from QML
Comment 1 Caio Marcelo de Oliveira Filho 2011-11-22 11:57:18 PST
For some context, see the thread https://lists.webkit.org/pipermail/webkit-qt/2011-October/001984.html
Comment 2 Caio Marcelo de Oliveira Filho 2011-11-22 12:02:05 PST
Created attachment 116260 [details]
Patch
Comment 3 Tor Arne Vestbø 2011-11-22 12:09:41 PST
Comment on attachment 116260 [details]
Patch

lgtm
Comment 4 Alexis Menard (darktears) 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).
Comment 5 Tor Arne Vestbø 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
Comment 6 Alexis Menard (darktears) 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
Comment 7 Caio Marcelo de Oliveira Filho 2011-11-22 13:11:58 PST
Created attachment 116268 [details]
Patch
Comment 8 Caio Marcelo de Oliveira Filho 2011-11-22 13:12:54 PST
This patch depends on bug 72959, that's why is not applying.
Comment 9 Alexis Menard (darktears) 2011-11-22 13:16:22 PST
Comment on attachment 116268 [details]
Patch

I prefer like this :D
Comment 10 Rafael Brandao 2011-11-22 14:17:24 PST
Comment on attachment 116268 [details]
Patch

LGTM :)
Comment 11 Caio Marcelo de Oliveira Filho 2011-11-23 06:49:27 PST
Created attachment 116352 [details]
Patch
Comment 12 Tor Arne Vestbø 2011-11-23 08:40:01 PST
Comment on attachment 116352 [details]
Patch

lgtm
Comment 13 Caio Marcelo de Oliveira Filho 2011-11-23 08:56:41 PST
Committed r101086: <http://trac.webkit.org/changeset/101086>