Bug 170862

Summary: Fix basic WKURLSchemeHandler bugs
Product: WebKit Reporter: Brady Eidson <beidson>
Component: WebKit2Assignee: Brady Eidson <beidson>
Status: RESOLVED FIXED    
Severity: Normal CC: aestes, buildbot, cdumez, commit-queue, dbates, japhet, ryanhaddad
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Brady Eidson 2017-04-14 15:29:41 PDT
Fix basic WKURLSchemeHandler bugs

For a couple of tiny little different reasons, the API doesn't really work.

Make it work, and add tests to verify that it does.
Comment 1 Brady Eidson 2017-04-14 15:30:11 PDT
<rdar://problem/30647559>
Comment 2 Brady Eidson 2017-04-14 15:35:12 PDT
Created attachment 307157 [details]
Patch
Comment 3 Andy Estes 2017-04-14 15:46:52 PDT
Comment on attachment 307157 [details]
Patch

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

> Source/WebKit2/UIProcess/Cocoa/NavigationState.mm:314
> -        tryAppLink(WTFMove(localNavigationAction), mainFrameURLString, [localListener, localNavigationAction = RefPtr<API::NavigationAction>(&navigationAction)] (bool followedLinkToApp) {
> +        tryAppLink(WTFMove(localNavigationAction), mainFrameURLString, [webPage = RefPtr<WebPageProxy>(&webPageProxy), localListener, localNavigationAction = RefPtr<API::NavigationAction>(&navigationAction)] (bool followedLinkToApp) {

Can webPage not be a Ref? I slightly prefer using makeRef(Ptr) since it's less wordy.

> Source/WebKit2/UIProcess/Cocoa/NavigationState.mm:326
> -            if ([NSURLConnection canHandleRequest:nsURLRequest.get()]) {
> +            if ([NSURLConnection canHandleRequest:nsURLRequest.get()] || webPage->urlSchemeHandlerForScheme(nsURLRequest.get().URL.scheme)) {

I slightly prefer [nsURLRequest URL].scheme to avoid the .get().

> Source/WebKit2/WebProcess/WebPage/WebURLSchemeHandlerTaskProxy.cpp:97
> +    if (m_coreLoader && m_coreLoader->reachedTerminalState())
> +        m_coreLoader = nullptr;

Seems weird that this function mutates the object.

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/WKURLSchemeHandler-1.mm:38
> +static bool receivedScriptMessage = false;

You don't need to explicitly initialize this to false.
Comment 4 Brady Eidson 2017-04-14 15:49:12 PDT
Created attachment 307159 [details]
Patch
Comment 5 Brady Eidson 2017-04-14 15:52:33 PDT
(In reply to Andy Estes from comment #3)
> Comment on attachment 307157 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=307157&action=review
> 
> > Source/WebKit2/UIProcess/Cocoa/NavigationState.mm:314
> > -        tryAppLink(WTFMove(localNavigationAction), mainFrameURLString, [localListener, localNavigationAction = RefPtr<API::NavigationAction>(&navigationAction)] (bool followedLinkToApp) {
> > +        tryAppLink(WTFMove(localNavigationAction), mainFrameURLString, [webPage = RefPtr<WebPageProxy>(&webPageProxy), localListener, localNavigationAction = RefPtr<API::NavigationAction>(&navigationAction)] (bool followedLinkToApp) {
> 
> Can webPage not be a Ref? I slightly prefer using makeRef(Ptr) since it's
> less wordy.

Nope, because this particular lambda must be copyable, and Ref<>s are not copyable.

> > Source/WebKit2/UIProcess/Cocoa/NavigationState.mm:326
> > -            if ([NSURLConnection canHandleRequest:nsURLRequest.get()]) {
> > +            if ([NSURLConnection canHandleRequest:nsURLRequest.get()] || webPage->urlSchemeHandlerForScheme(nsURLRequest.get().URL.scheme)) {
> 
> I slightly prefer [nsURLRequest URL].scheme to avoid the .get().

Done.

> > Source/WebKit2/WebProcess/WebPage/WebURLSchemeHandlerTaskProxy.cpp:97
> > +    if (m_coreLoader && m_coreLoader->reachedTerminalState())
> > +        m_coreLoader = nullptr;
> 
> Seems weird that this function mutates the object.

Indeed. (I did not change this)

> > Tools/TestWebKitAPI/Tests/WebKit2Cocoa/WKURLSchemeHandler-1.mm:38
> > +static bool receivedScriptMessage = false;
> 
> You don't need to explicitly initialize this to false.

Indeed. (I changed this)
Comment 6 Brady Eidson 2017-04-14 15:56:40 PDT
Created attachment 307161 [details]
Patch
Comment 7 WebKit Commit Bot 2017-04-14 17:17:15 PDT
Comment on attachment 307161 [details]
Patch

Rejecting attachment 307161 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 307161, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
.webkit.org/git/WebKit
   7f37bac..c86e411  master     -> origin/master
Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ...
Currently at 215380 = 7f37bacfa28a61a81a6c4e1498a6e357de83bba6
r215381 = c86e41118bce64523a2092fa5bbfcba2d6c24bc9
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.

Full output: http://webkit-queues.webkit.org/results/3536570
Comment 8 WebKit Commit Bot 2017-04-14 17:52:14 PDT
Comment on attachment 307161 [details]
Patch

Clearing flags on attachment: 307161

Committed r215384: <http://trac.webkit.org/changeset/215384>
Comment 9 WebKit Commit Bot 2017-04-14 17:52:15 PDT
All reviewed patches have been landed.  Closing bug.