Bug 72976 - [Qt] [WK2] Expose onNavigationRequested signal instead of expecting a slot be defined from QML
: [Qt] [WK2] Expose onNavigationRequested signal instead of expecting a slot be...
Status: RESOLVED FIXED
: WebKit
New Bugs
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
: 72959
:
  Show dependency treegraph
 
Reported: 2011-11-22 11:30 PST by
Modified: 2011-11-23 08:56 PST (History)


Attachments
Patch (16.22 KB, patch)
2011-11-22 12:02 PST, Caio Marcelo de Oliveira Filho
no flags Review Patch | Details | Formatted Diff | Diff
Patch (17.67 KB, patch)
2011-11-22 13:11 PST, Caio Marcelo de Oliveira Filho
no flags Review Patch | Details | Formatted Diff | Diff
Patch (17.70 KB, patch)
2011-11-23 06:49 PST, Caio Marcelo de Oliveira Filho
vestbo: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-11-22 11:30:26 PST
[Qt] [WK2] Expose onNavigationRequested signal instead of expecting a slot be defined from QML
------- Comment #1 From 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 From 2011-11-22 12:02:05 PST -------
Created an attachment (id=116260) [details]
Patch
------- Comment #3 From 2011-11-22 12:09:41 PST -------
(From update of attachment 116260 [details])
lgtm
------- Comment #4 From 2011-11-22 12:21:32 PST -------
(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).
------- Comment #5 From 2011-11-22 12:26:11 PST -------
(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
------- Comment #6 From 2011-11-22 12:31:34 PST -------
(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 116260 [details] [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 From 2011-11-22 13:11:58 PST -------
Created an attachment (id=116268) [details]
Patch
------- Comment #8 From 2011-11-22 13:12:54 PST -------
This patch depends on bug 72959, that's why is not applying.
------- Comment #9 From 2011-11-22 13:16:22 PST -------
(From update of attachment 116268 [details])
I prefer like this :D
------- Comment #10 From 2011-11-22 14:17:24 PST -------
(From update of attachment 116268 [details])
LGTM :)
------- Comment #11 From 2011-11-23 06:49:27 PST -------
Created an attachment (id=116352) [details]
Patch
------- Comment #12 From 2011-11-23 08:40:01 PST -------
(From update of attachment 116352 [details])
lgtm
------- Comment #13 From 2011-11-23 08:56:41 PST -------
Committed r101086: <http://trac.webkit.org/changeset/101086>