| Summary: | Add a new 'limitNavigationsToAppBoundDomains' property to WKWebViewConfiguration | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Brent Fulgham <bfulgham> | ||||||||
| Component: | WebKit2 | Assignee: | Brent Fulgham <bfulgham> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | aestes, bfulgham, katherine_cheney, mjs, webkit-bug-importer, wilander | ||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||
| Version: | Safari 13 | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Bug Depends on: | |||||||||||
| Bug Blocks: | 211393, 230190 | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Brent Fulgham
2020-04-16 15:08:36 PDT
Created attachment 396710 [details]
Patch
I think this looks good. Comment on attachment 396710 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=396710&action=review > Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfiguration.mm:1256 > + _limitsNavigationToAppBoundDomains = !ignoresAppBoundDomains; Seems odd that setting one configuration property has the side effect of changing another one. If these two properties are mutually exclusive, should have a single enum property instead with possible values of `ignoresAppBoundDomains` and `limitsToAppBoundDomains`? Comment on attachment 396710 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=396710&action=review >> Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfiguration.mm:1256 >> + _limitsNavigationToAppBoundDomains = !ignoresAppBoundDomains; > > Seems odd that setting one configuration property has the side effect of changing another one. If these two properties are mutually exclusive, should have a single enum property instead with possible values of `ignoresAppBoundDomains` and `limitsToAppBoundDomains`? One is internal SPI that is used to gate several things. I just wanted to make sure that when the SPI was used, this new flag wasn't left in an inconsistent state. Comment on attachment 396710 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=396710&action=review >>> Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfiguration.mm:1256 >>> + _limitsNavigationToAppBoundDomains = !ignoresAppBoundDomains; >> >> Seems odd that setting one configuration property has the side effect of changing another one. If these two properties are mutually exclusive, should have a single enum property instead with possible values of `ignoresAppBoundDomains` and `limitsToAppBoundDomains`? > > One is internal SPI that is used to gate several things. I just wanted to make sure that when the SPI was used, this new flag wasn't left in an inconsistent state. But this just changes the value reflected by the API, not how it's enforced (but maybe that's for a follow-up). Anyway, I'd be in favor of not changing a value set by the API/SPI client but just documenting what the behavior is when there is a conflict between the two configuration settings. Committed r260228: <https://trac.webkit.org/changeset/260228> All reviewed patches have been landed. Closing bug and clearing flags on attachment 396710 [details]. Reopening because I realized the setter wasn't correct. Created attachment 396729 [details]
Patch for landing
Comment on attachment 396710 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=396710&action=review >>>> Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfiguration.mm:1256 >>>> + _limitsNavigationToAppBoundDomains = !ignoresAppBoundDomains; >>> >>> Seems odd that setting one configuration property has the side effect of changing another one. If these two properties are mutually exclusive, should have a single enum property instead with possible values of `ignoresAppBoundDomains` and `limitsToAppBoundDomains`? >> >> One is internal SPI that is used to gate several things. I just wanted to make sure that when the SPI was used, this new flag wasn't left in an inconsistent state. > > But this just changes the value reflected by the API, not how it's enforced (but maybe that's for a follow-up). Anyway, I'd be in favor of not changing a value set by the API/SPI client but just documenting what the behavior is when there is a conflict between the two configuration settings. I'll land a follow-up fix addressing that. Created attachment 396730 [details]
Patch for landing
Committed r260232: <https://trac.webkit.org/changeset/260232> All reviewed patches have been landed. Closing bug and clearing flags on attachment 396730 [details]. Patch 396729 does not build |