[webkit-reviews] review granted: [Bug 170862] Fix basic WKURLSchemeHandler bugs : [Attachment 307157] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Apr 14 15:46:52 PDT 2017
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.
More information about the webkit-reviews
mailing list