Summary: | AppBound domain behavior should abide by the limitsNavigationsToAppBoundDomains argument in WKWebView configuration | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kate Cheney <katherine_cheney> | ||||||
Component: | WebKit Misc. | Assignee: | Kate Cheney <katherine_cheney> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | aestes, bfulgham, wilander | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Kate Cheney
2020-04-20 14:59:14 PDT
Created attachment 397024 [details]
Patch
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. 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. (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. :-) 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! 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! Created attachment 397045 [details]
Patch for landing
Committed r260408: <https://trac.webkit.org/changeset/260408> All reviewed patches have been landed. Closing bug and clearing flags on attachment 397045 [details]. 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. |