It is wasteful to check the app-bound domain list and wait for a response when some protocols are automatically considered app-bound.
<rdar://problem/60875376>
Created attachment 394936 [details] Patch
Created attachment 394937 [details] Patch
Comment on attachment 394937 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394937&action=review > Source/WebKit/UIProcess/WebPageProxy.cpp:5115 > +#if PLATFORM(COCOA) && PLATFORM(IOS_FAMILY) PLATFORM(COCOA) && seems redundant > Source/WebKit/UIProcess/WebPageProxy.cpp:5116 > + if (!shouldBeTreatedAsAppBound(request.url())) Not sure I understand why the ! here.
Comment on attachment 394937 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394937&action=review >> Source/WebKit/UIProcess/WebPageProxy.cpp:5115 >> +#if PLATFORM(COCOA) && PLATFORM(IOS_FAMILY) > > PLATFORM(COCOA) && seems redundant Removing. >> Source/WebKit/UIProcess/WebPageProxy.cpp:5116 >> + if (!shouldBeTreatedAsAppBound(request.url())) > > Not sure I understand why the ! here. If shouldExpectAppBoundDomainResult is Yes, we make the call to check the app-bound domain list (see below). We only want to check the app-bound domain list if the request url is not one of these special, automatic app-bound domain protocols. Keeping it as ShouldExpectAppBoundDomainResult::No for the app-bound domain protocols makes sure we don't dispatch and wait for the list to be checked. ShouldExpectAppBoundDomainResult::No now automatically returns NavigatingToAppBoundDomain::Yes in the WebFramePolicyListener constructor.
(In reply to katherine_cheney from comment #5) > Comment on attachment 394937 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=394937&action=review > > >> Source/WebKit/UIProcess/WebPageProxy.cpp:5115 > >> +#if PLATFORM(COCOA) && PLATFORM(IOS_FAMILY) > > > > PLATFORM(COCOA) && seems redundant > > Removing. > > >> Source/WebKit/UIProcess/WebPageProxy.cpp:5116 > >> + if (!shouldBeTreatedAsAppBound(request.url())) > > > > Not sure I understand why the ! here. > > If shouldExpectAppBoundDomainResult is Yes, we make the call to check the > app-bound domain list (see below). We only want to check the app-bound > domain list if the request url is not one of these special, automatic > app-bound domain protocols. > By "see below," I meant this line in the patch: + if (shouldExpectAppBoundDomainResult == ShouldExpectAppBoundDomainResult::Yes) + m_websiteDataStore->beginAppBoundDomainCheck(RegistrableDomain { request.url() }, listener); > Keeping it as ShouldExpectAppBoundDomainResult::No for the app-bound domain > protocols makes sure we don't dispatch and wait for the list to be checked. > ShouldExpectAppBoundDomainResult::No now automatically returns > NavigatingToAppBoundDomain::Yes in the WebFramePolicyListener constructor.
Created attachment 394952 [details] Patch
While I believe you that the patch does the right thing, it seems to do the right thing in a confusing way and it would be nice to refactor the code so that it is clearer. Right now, at best it would need a comment. But if code requires a comment, then it likely means the code is not clear enough to be self documenting.
Comment on attachment 394952 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394952&action=review > Source/WebKit/UIProcess/WebPageProxy.cpp:-5177 > - m_websiteDataStore->beginAppBoundDomainCheck(RegistrableDomain { request.url() }, listener); Can beginAppBoundDomainCheck() take a URL and do an early return if the protocol is definitely app bound? Seems like it might be clearer than all this special casing at the call site. We'd always do an app bound check but the check would be super cheap in some cases.
Comment on attachment 394952 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394952&action=review >> Source/WebKit/UIProcess/WebPageProxy.cpp:-5177 >> - m_websiteDataStore->beginAppBoundDomainCheck(RegistrableDomain { request.url() }, listener); > > Can beginAppBoundDomainCheck() take a URL and do an early return if the protocol is definitely app bound? Seems like it might be clearer than all this special casing at the call site. We'd always do an app bound check but the check would be super cheap in some cases. Also, please rename shouldBeTreatedAsAppBound() to something like shouldTreatURLProtocolAsAppBound()
Created attachment 394961 [details] Patch
Comment on attachment 394961 [details] Patch me likee
(In reply to Chris Dumez from comment #12) > Comment on attachment 394961 [details] > Patch > > me likee :) thanks!
Committed r259240: <https://trac.webkit.org/changeset/259240> All reviewed patches have been landed. Closing bug and clearing flags on attachment 394961 [details].