Bug 145417 – Add a "should open urls externally" flag to DocumentLoader

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Brady Eidson]

Add a "should open urls externally" flag to DocumentLoader.

The flag will optionally be set by the WebKit client in a loading API
The flag will propagate down to the DocumentLoader via FrameLoadRequest.
The flag will bubble back out to WebKit navigation delegate API for policy decisions.

As far as WebCore/WebKit is concerned, this patch will not introduce any behavior change.

As discussed in blocked bug https://bugs.webkit.org/show_bug.cgi?id=145280, the followup patch will make more extensive changes to WebCore and will be testable.

[Brady Eidson]

Created attachment 253810 [details]
Patch v1

[WebKit Commit Bot]

Attachment 253810 [details] did not pass style-queue:


ERROR: Source/WebCore/loader/DocumentLoader.h:181:  The parameter name "action" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:78:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebKit2/UIProcess/WebPageProxy.h:355:  The parameter name "openURLsExternallyPolicy" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKBrowsingContextController.mm:48:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 4 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Geoffrey Garen]

Comment on attachment 253810 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=253810&action=review

I'm a bit concerned that we have three phrases for the same thing:
_web_isUserInitiated
OpenURLsExternallyPolicy
shouldOpenURLsExternally

Can we get this down to one phrase?

> Source/WebCore/loader/DocumentLoader.cpp:1560
> +        m_triggeringAction.setOpenURLsExternallyPolicy(m_openURLsExternallyPolicy);

When I first read "shouldOpenURLsExternally", it struck me as saying "all URLs should open externally", which wasn't your intent. 

shouldOpenExternalURLs?

[Brady Eidson]

(In reply to comment #3)
> Comment on attachment 253810 [details]
> Patch v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=253810&action=review
> 
> I'm a bit concerned that we have three phrases for the same thing:
> _web_isUserInitiated
> OpenURLsExternallyPolicy
> shouldOpenURLsExternally
> 
> Can we get this down to one phrase?

Maybe.

> When I first read "shouldOpenURLsExternally", it struck me as saying "all
> URLs should open externally", which wasn't your intent. 
> 
> shouldOpenExternalURLs?

ShouldOpenExternalURLsPolicy for internal use, shouldOpenExternalURLs for the API layer?

[Brady Eidson]

Created attachment 253818 [details]
Patch for EWS/Landing

[WebKit Commit Bot]

Comment on attachment 253818 [details]
Patch for EWS/Landing

Rejecting attachment 253818 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 253818, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
ubmit
    return self.open(self.click(*args, **kwds))
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_mechanize.py", line 203, in open
    return self._mech_open(url, data, timeout=timeout)
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_mechanize.py", line 255, in _mech_open
    raise response
webkitpy.thirdparty.autoinstalled.mechanize._response.httperror_seek_wrapper: HTTP Error 500: Internal Server Error

Full output: http://webkit-queues.appspot.com/results/5598173376544768

[Brady Eidson]

(In reply to comment #6)
> Comment on attachment 253818 [details]
> Patch for EWS/Landing
> 
> Rejecting attachment 253818 [details] from commit-queue.
> 
> Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch',
> '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-01',
> 'apply-attachment', '--no-update', '--non-interactive', 253818,
> '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit
> 
> Last 500 characters of output:
> ubmit
>     return self.open(self.click(*args, **kwds))
>   File
> "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/thirdparty/autoinstalled/
> mechanize/_mechanize.py", line 203, in open
>     return self._mech_open(url, data, timeout=timeout)
>   File
> "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/thirdparty/autoinstalled/
> mechanize/_mechanize.py", line 255, in _mech_open
>     raise response
> webkitpy.thirdparty.autoinstalled.mechanize._response.httperror_seek_wrapper:
> HTTP Error 500: Internal Server Error
> 
> Full output: http://webkit-queues.appspot.com/results/5598173376544768


This clearly looks like bogusness on the bot side.

[Brady Eidson]

http://trac.webkit.org/changeset/184941