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

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Caio Marcelo de Oliveira Filho]

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

[Caio Marcelo de Oliveira Filho]

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

[Caio Marcelo de Oliveira Filho]

Created attachment 116260 [details]
Patch

[Tor Arne Vestbø]

Comment on attachment 116260 [details]
Patch

lgtm

[Alexis Menard (darktears)]

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ø]

(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)]

(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]

Created attachment 116268 [details]
Patch

[Caio Marcelo de Oliveira Filho]

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

[Alexis Menard (darktears)]

Comment on attachment 116268 [details]
Patch

I prefer like this :D

[Rafael Brandao]

Comment on attachment 116268 [details]
Patch

LGTM :)

[Caio Marcelo de Oliveira Filho]

Created attachment 116352 [details]
Patch

[Tor Arne Vestbø]

Comment on attachment 116352 [details]
Patch

lgtm

[Caio Marcelo de Oliveira Filho]

Committed r101086: <http://trac.webkit.org/changeset/101086>