Bug 209755 – Known app-bound domain protocols should not check the app-bound domain list

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Kate Cheney]

It is wasteful to check the app-bound domain list and wait for a response when some protocols are automatically considered app-bound.

[Kate Cheney]

<rdar://problem/60875376>

[Kate Cheney]

Created attachment 394936 [details]
Patch

[Kate Cheney]

Created attachment 394937 [details]
Patch

[Chris Dumez]

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.

[Kate Cheney]

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.

[Kate Cheney]

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

[Kate Cheney]

Created attachment 394952 [details]
Patch

[Chris Dumez]

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.

[Chris Dumez]

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.

[Chris Dumez]

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

[Kate Cheney]

Created attachment 394961 [details]
Patch

[Chris Dumez]

Comment on attachment 394961 [details]
Patch

me likee

[Kate Cheney]

(In reply to Chris Dumez from comment #12)
> Comment on attachment 394961 [details]
> Patch
> 
> me likee

:) thanks!

[EWS]

Committed r259240: <https://trac.webkit.org/changeset/259240>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 394961 [details].