Bug 170862 – Fix basic WKURLSchemeHandler bugs

• Component: WebKit2
• Status: RESOLVED FIXED

[Brady Eidson]

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.

[Brady Eidson]

<rdar://problem/30647559>

[Brady Eidson]

Created attachment 307157 [details]
Patch

[Andy Estes]

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.

[Brady Eidson]

Created attachment 307159 [details]
Patch

[Brady Eidson]

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

[Brady Eidson]

Created attachment 307161 [details]
Patch

[WebKit Commit Bot]

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

[WebKit Commit Bot]

Comment on attachment 307161 [details]
Patch

Clearing flags on attachment: 307161

Committed r215384: <http://trac.webkit.org/changeset/215384>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.