Bug 210769 – AppBound domain behavior should abide by the limitsNavigationsToAppBoundDomains argument in WKWebView configuration

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

[Kate Cheney]

The limitsNavigationsToAppBoundDomains flag in the WKWebView configuration should opt a WebView into AppBound domain behavior and limit navigations to the domains in the Info.plist

[Kate Cheney]

<rdar://problem/62065241>

[Kate Cheney]

Created attachment 397024 [details]
Patch

[Kate Cheney]

I also think we can eventually clean this up and merge m_isNavigatingToAppBoundDomain and m_hasNavigatedAwayFromAppBoundDomain into one Optional parameter that defaults to WTF::nullopt. But this would be a large refactoring and is probably better for a future patch.

[Brent Fulgham]

Comment on attachment 397024 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=397024&action=review

Looks good. r=me, but please consider my suggestion for renaming.

> Source/WebKit/UIProcess/WebPageProxy.cpp:3130
> +bool WebPageProxy::setIsNavigatingToAppBoundDomain(bool isMainFrame, const URL& requestURL, Optional<NavigatingToAppBoundDomain> isNavigatingToAppBoundDomain)

I think this would be clearer with a name change: Maybe something like:

bool WebPageProxy::setIsNavigatingToAppBoundDomainAndCheckIfPermitted(...)

> Source/WebKit/UIProcess/WebPageProxy.cpp:3144
> +            if (*isNavigatingToAppBoundDomain == NavigatingToAppBoundDomain::Yes) {

I would suggest reversing this check, and doing an early return:

if (*isNavigatingToAppBoundDomain == NavigatingToAppBoundDomain::No)
    return false;

Also: If we make the decision to return early here, should we be setting "m_hasNavigatedAwayFromAppBoundDomain = NavigatedAwayFromAppBoundDomain::No"?

> Source/WebKit/UIProcess/WebPageProxy.cpp:3150
>          }

Maybe this could then be an 'else' block, and both fall through to the 'true' at the bottom.

[Brent Fulgham]

(In reply to katherine_cheney from comment #3)
> I also think we can eventually clean this up and merge
> m_isNavigatingToAppBoundDomain and m_hasNavigatedAwayFromAppBoundDomain into
> one Optional parameter that defaults to WTF::nullopt. But this would be a
> large refactoring and is probably better for a future patch.

Yes -- this sounds like a good next step. Might simplify the logic and make it easier to understand. :-)

[Kate Cheney]

Comment on attachment 397024 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=397024&action=review

>> Source/WebKit/UIProcess/WebPageProxy.cpp:3130
>> +bool WebPageProxy::setIsNavigatingToAppBoundDomain(bool isMainFrame, const URL& requestURL, Optional<NavigatingToAppBoundDomain> isNavigatingToAppBoundDomain)
> 
> I think this would be clearer with a name change: Maybe something like:
> 
> bool WebPageProxy::setIsNavigatingToAppBoundDomainAndCheckIfPermitted(...)

Good idea, I'll do this

>> Source/WebKit/UIProcess/WebPageProxy.cpp:3144
>> +            if (*isNavigatingToAppBoundDomain == NavigatingToAppBoundDomain::Yes) {
> 
> I would suggest reversing this check, and doing an early return:
> 
> if (*isNavigatingToAppBoundDomain == NavigatingToAppBoundDomain::No)
>     return false;
> 
> Also: If we make the decision to return early here, should we be setting "m_hasNavigatedAwayFromAppBoundDomain = NavigatedAwayFromAppBoundDomain::No"?

Early return sounds good.

m_hasNavigatedAwayFromAppBoundDomain defaults to No, and won't ever be changed to Yes for a webView with the m_limitsNavigationsToAppBoundDomains flag, so it should always be the case that m_hasNavigatedAwayFromAppBoundDomain = NavigatedAwayFromAppBoundDomain::No, and we shouldn't need to set it.

>> Source/WebKit/UIProcess/WebPageProxy.cpp:3150
>>          }
> 
> Maybe this could then be an 'else' block, and both fall through to the 'true' at the bottom.

Will do!

[Kate Cheney]

Comment on attachment 397024 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=397024&action=review

>> Source/WebKit/UIProcess/WebPageProxy.cpp:3130
>> +bool WebPageProxy::setIsNavigatingToAppBoundDomain(bool isMainFrame, const URL& requestURL, Optional<NavigatingToAppBoundDomain> isNavigatingToAppBoundDomain)
> 
> I think this would be clearer with a name change: Maybe something like:
> 
> bool WebPageProxy::setIsNavigatingToAppBoundDomainAndCheckIfPermitted(...)

Good idea, I'll do this

>> Source/WebKit/UIProcess/WebPageProxy.cpp:3144
>> +            if (*isNavigatingToAppBoundDomain == NavigatingToAppBoundDomain::Yes) {
> 
> I would suggest reversing this check, and doing an early return:
> 
> if (*isNavigatingToAppBoundDomain == NavigatingToAppBoundDomain::No)
>     return false;
> 
> Also: If we make the decision to return early here, should we be setting "m_hasNavigatedAwayFromAppBoundDomain = NavigatedAwayFromAppBoundDomain::No"?

Early return sounds good.

m_hasNavigatedAwayFromAppBoundDomain defaults to No, and won't ever be changed to Yes for a webView with the m_limitsNavigationsToAppBoundDomains flag, so it should always be the case that m_hasNavigatedAwayFromAppBoundDomain = NavigatedAwayFromAppBoundDomain::No, and we shouldn't need to set it.

>> Source/WebKit/UIProcess/WebPageProxy.cpp:3150
>>          }
> 
> Maybe this could then be an 'else' block, and both fall through to the 'true' at the bottom.

Will do!

[Kate Cheney]

Created attachment 397045 [details]
Patch for landing

[EWS]

Committed r260408: <https://trac.webkit.org/changeset/260408>

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

[Brent Fulgham]

Comment on attachment 397024 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=397024&action=review

>>>> Source/WebKit/UIProcess/WebPageProxy.cpp:3144
>>>> +            if (*isNavigatingToAppBoundDomain == NavigatingToAppBoundDomain::Yes) {
>>> 
>>> I would suggest reversing this check, and doing an early return:
>>> 
>>> if (*isNavigatingToAppBoundDomain == NavigatingToAppBoundDomain::No)
>>>     return false;
>>> 
>>> Also: If we make the decision to return early here, should we be setting "m_hasNavigatedAwayFromAppBoundDomain = NavigatedAwayFromAppBoundDomain::No"?
>> 
>> Early return sounds good.
>> 
>> m_hasNavigatedAwayFromAppBoundDomain defaults to No, and won't ever be changed to Yes for a webView with the m_limitsNavigationsToAppBoundDomains flag, so it should always be the case that m_hasNavigatedAwayFromAppBoundDomain = NavigatedAwayFromAppBoundDomain::No, and we shouldn't need to set it.
> 
> Early return sounds good.
> 
> m_hasNavigatedAwayFromAppBoundDomain defaults to No, and won't ever be changed to Yes for a webView with the m_limitsNavigationsToAppBoundDomains flag, so it should always be the case that m_hasNavigatedAwayFromAppBoundDomain = NavigatedAwayFromAppBoundDomain::No, and we shouldn't need to set it.

Ah -- okay. I didn't understand that.