Bug 210620 - Add a new 'limitNavigationsToAppBoundDomains' property to WKWebViewConfiguration
Summary: Add a new 'limitNavigationsToAppBoundDomains' property to WKWebViewConfiguration
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: Safari 13
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords: InRadar
Depends on:
Blocks: 211393
  Show dependency treegraph
 
Reported: 2020-04-16 15:08 PDT by Brent Fulgham
Modified: 2020-05-07 17:08 PDT (History)
6 users (show)

See Also:


Attachments
Patch (9.21 KB, patch)
2020-04-16 15:37 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch for landing (3.47 KB, patch)
2020-04-16 17:41 PDT, Brent Fulgham
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (3.47 KB, patch)
2020-04-16 17:45 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2020-04-16 15:08:36 PDT
Update WKWebViewConfiguration, APIPageConfiguration, and WebPageProxy with a new flag indicating that the Developer would like to limit navigations of this web view to the set of app-bound domains specified in the Info.plist for the application.
Comment 1 Brent Fulgham 2020-04-16 15:08:55 PDT
<rdar://problem/61903225>
Comment 2 Brent Fulgham 2020-04-16 15:37:06 PDT
Created attachment 396710 [details]
Patch
Comment 3 katherine_cheney 2020-04-16 15:59:04 PDT
I think this looks good.
Comment 4 Andy Estes 2020-04-16 16:04:23 PDT
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 5 Brent Fulgham 2020-04-16 16:34:04 PDT
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 6 Andy Estes 2020-04-16 16:43:32 PDT
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.
Comment 7 EWS 2020-04-16 17:05:54 PDT
Committed r260228: <https://trac.webkit.org/changeset/260228>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 396710 [details].
Comment 8 Brent Fulgham 2020-04-16 17:29:35 PDT
Reopening because I realized the setter wasn't correct.
Comment 9 Brent Fulgham 2020-04-16 17:41:31 PDT
Created attachment 396729 [details]
Patch for landing
Comment 10 Brent Fulgham 2020-04-16 17:42:31 PDT
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.
Comment 11 Brent Fulgham 2020-04-16 17:45:51 PDT
Created attachment 396730 [details]
Patch for landing
Comment 12 EWS 2020-04-16 18:09:35 PDT
Committed r260232: <https://trac.webkit.org/changeset/260232>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 396730 [details].
Comment 13 EWS 2020-04-16 18:11:30 PDT
Patch 396729 does not build