Summary: | Fix basic WKURLSchemeHandler bugs | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Brady Eidson <beidson> | ||||||||
Component: | WebKit2 | Assignee: | 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
Brady Eidson
2017-04-14 15:29:41 PDT
Created attachment 307157 [details]
Patch
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. Created attachment 307159 [details]
Patch
(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) Created attachment 307161 [details]
Patch
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 on attachment 307161 [details] Patch Clearing flags on attachment: 307161 Committed r215384: <http://trac.webkit.org/changeset/215384> All reviewed patches have been landed. Closing bug. |