[webkit-reviews] review denied: [Bug 208461] Add flag to indicate that ITP state was explicitly set : [Attachment 392172] Patch

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has denied Brent Fulgham <bfulgham at webkit.org>'s
request for review:
Bug 208461: Add flag to indicate that ITP state was explicitly set
https://bugs.webkit.org/show_bug.cgi?id=208461

Attachment 392172: Patch

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




--- Comment #4 from Sam Weinig <sam at webkit.org> ---
Comment on attachment 392172
  --> https://bugs.webkit.org/attachment.cgi?id=392172
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.