Bug 208461

<rdar://problem/59960829>

Created attachment 392172 [details]
Patch

Comment on attachment 392172 [details]
Patch

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

r=me with comments and fixes for the non-Cocoa builds.

> Source/WebKit/ChangeLog:10
> +        This patch takes the first step by adding a flag to the NetworkSessionCreationParameters

I try to always provide the namespace when reasoning about classes in text, in this case WebKit::NetworkSessionCreationParameters.

> Source/WebKit/ChangeLog:14
> +        Subsequent patches will enable ITP in Ephmeral sessions, allowing us to decouple

Landed in https://trac.webkit.org/changeset/257726.

> Source/WebKit/NetworkProcess/NetworkSessionCreationParameters.cpp:183
> +    Optional<bool> itpStateWasExplicitlySet;

Since it's a boolean, I think we should go with present tense and lead with it, like isITPStateExplicitlySet.

> Source/WebKit/NetworkProcess/NetworkSessionCreationParameters.h:85
> +    bool itpStateWasExplicitlySet { false };

Ditto.

> Source/WebKit/NetworkProcess/mac/com.apple.WebKit.NetworkProcess.sb.in:443
> +;; Needed for TCC

Missing period.

> Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.Networking.sb:678
> +;; Needed for TCC

Ditto.

> Source/WebKit/Scripts/process-entitlements.sh:48
> +        if (( "${TARGET_MAC_OS_X_VERSION_MAJOR}" >= 101600 ))

I assume nothing similar is needed for other platforms.

> Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:165
> +        itpStateWasExplicitlySet(),

isITPStateExplicitlySet()

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h:198
> +    bool itpStateWasExplicitlySet() const { return m_itpStateWasExplicitlySet; }

isITPStateExplicitlySet()

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h:341
> +    bool m_itpStateWasExplicitlySet { false };

m_isITPStateExplicitlySet

Since we're capturing something explicit, is there a possible enum here? Like { implicitlyOff, implicitlyOn, explicitlyOff, explicitlyOn } ? I'm thinking if there's a way to capture the ITP on/off and explicitlySet Yes/No in one, self-documenting flag.

Comment on attachment 392172 [details]
Patch

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

> Source/WebKit/ChangeLog:9
> +        We would like to move to a more global concept of ITP being on or off.

Global in what sense? Why is this desirable?

> Source/WebKit/ChangeLog:15
> +        Subsequent patches will enable ITP in Ephmeral sessions, allowing us to decouple
> +        the ITP state from the WebsiteDataStore.

Why is this desirable? As ITP state is website data, it seems like coupling it with WebsiteDataStore is the right design.

> Source/WebKit/ChangeLog:18
> +        This patch also lays some groundwork for communicating with TCC for the purpose of
> +        checking if ITP is on or off, though it does not actually use this knowledge yet.

This seems unrelated to the change indicated in the title, and should be done in a different patch.

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:1165
> +    if (!parameters.itpStateWasExplicitlySet)
> +        activateSessionCleanup(*this);

I think this deserves a comment explaining why cleanup doesn't happen when this flag is set. 

That said, how is this call to activateSessionCleanup() being tested now that it's not being called when itpStateWasExplicitlySet is called?

> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:72
> +    websiteDataStore->useExplicitITPState();

From the name, it seems like "useExplicitITPState" would come along with some explicit state being used. But instead, it seems to be indicating that ITP is enabled. I think the terminology needs to be clearer here.

Comment on attachment 392172 [details]
Patch

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

>> Source/WebKit/ChangeLog:9
>> +        We would like to move to a more global concept of ITP being on or off.
> 
> Global in what sense? Why is this desirable?

We no longer want a model where ITP is on for some WKWebViews, but not others. It no longer necessary to track this on a WebsiteDataStore level since this setting should be honored across all web views in the process.

>> Source/WebKit/ChangeLog:10
>> +        This patch takes the first step by adding a flag to the NetworkSessionCreationParameters
> 
> I try to always provide the namespace when reasoning about classes in text, in this case WebKit::NetworkSessionCreationParameters.

Will do!

>> Source/WebKit/ChangeLog:14
>> +        Subsequent patches will enable ITP in Ephmeral sessions, allowing us to decouple
> 
> Landed in https://trac.webkit.org/changeset/257726.

Oh, great!

>> Source/WebKit/NetworkProcess/NetworkSessionCreationParameters.cpp:183
>> +    Optional<bool> itpStateWasExplicitlySet;
> 
> Since it's a boolean, I think we should go with present tense and lead with it, like isITPStateExplicitlySet.

Will do.

>> Source/WebKit/NetworkProcess/mac/com.apple.WebKit.NetworkProcess.sb.in:443
>> +;; Needed for TCC
> 
> Missing period.

Will do.

>> Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.Networking.sb:678
>> +;; Needed for TCC
> 
> Ditto.

Will do.

>> Source/WebKit/Scripts/process-entitlements.sh:48
>> +        if (( "${TARGET_MAC_OS_X_VERSION_MAJOR}" >= 101600 ))
> 
> I assume nothing similar is needed for other platforms.

Correct. They don't build down-level versions, so we don't need this check.

>> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:72
>> +    websiteDataStore->useExplicitITPState();
> 
> From the name, it seems like "useExplicitITPState" would come along with some explicit state being used. But instead, it seems to be indicating that ITP is enabled. I think the terminology needs to be clearer here.

ITP could be enabled or disabled through this call. This flag notes that WebsiteDataStore's ITP value is the result of an explicit SPI call being made.

>> Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:165
>> +        itpStateWasExplicitlySet(),
> 
> isITPStateExplicitlySet()

Will do.

Created attachment 392213 [details]
Patch

Created attachment 392227 [details]
Patch

Comment on attachment 392227 [details]
Patch

LGTM. Leaving the cq? in wait for a green light on the wpe and api-gtk bots.

(In reply to John Wilander from comment #8)
> Comment on attachment 392227 [details]
> Patch
> 
> LGTM. Leaving the cq? in wait for a green light on the wpe and api-gtk bots.

wpe is now green.... just waiting for api-gtk now.

I believe the absence of this patch is breaking the internal Apple bots

Comment on attachment 392227 [details]
Patch

Clearing flags on attachment: 392227

Committed r257758: <https://trac.webkit.org/changeset/257758>

All reviewed patches have been landed.  Closing bug.