[webkit-reviews] review granted: [Bug 210769] AppBound domain behavior should abide by the limitsNavigationsToAppBoundDomains argument in WKWebView configuration : [Attachment 397024] Patch

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has granted katherine_cheney at apple.com's
request for review:
Bug 210769: AppBound domain behavior should abide by the
limitsNavigationsToAppBoundDomains argument in WKWebView configuration
https://bugs.webkit.org/show_bug.cgi?id=210769

Attachment 397024: Patch

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




--- Comment #4 from Brent Fulgham <bfulgham at webkit.org> ---
Comment on attachment 397024
  --> https://bugs.webkit.org/attachment.cgi?id=397024
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.