[webkit-reviews] review granted: [Bug 170862] Fix basic WKURLSchemeHandler bugs : [Attachment 307157] Patch

[bugzilla-daemon at webkit.org]

Andy Estes <aestes at apple.com> has granted Brady Eidson <beidson at apple.com>'s
request for review:
Bug 170862: Fix basic WKURLSchemeHandler bugs
https://bugs.webkit.org/show_bug.cgi?id=170862

Attachment 307157: Patch

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




--- Comment #3 from Andy Estes <aestes at apple.com> ---
Comment on attachment 307157
  --> https://bugs.webkit.org/attachment.cgi?id=307157
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.