[webkit-reviews] review denied: [Bug 197108] Disable Ad Click Attribution in ephemeral sessions and make sure conversion requests use an ephemeral, stateless session : [Attachment 367830] Patch

[bugzilla-daemon at webkit.org]

Alex Christensen <achristensen at apple.com> has denied John Wilander
<wilander at apple.com>'s request for review:
Bug 197108: Disable Ad Click Attribution in ephemeral sessions and make sure
conversion requests use an ephemeral, stateless session
https://bugs.webkit.org/show_bug.cgi?id=197108

Attachment 367830: Patch

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




--- Comment #3 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 367830
  --> https://bugs.webkit.org/attachment.cgi?id=367830
Patch

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

Seems mostly conceptually good to me.

> Source/WebCore/loader/ResourceLoader.cpp:721
> +    if (m_options.storedCredentialsPolicy ==
StoredCredentialsPolicy::DoNotUse
> +	   || m_options.storedCredentialsPolicy ==
StoredCredentialsPolicy::EphemeralStatelessCookieless)

This could just be if it's not equal to ::Use.

> Source/WebKit/NetworkProcess/AdClickAttributionManager.cpp:183
> -	   return completionHandler("No stored Ad Click Attribution
data.\n"_s);
> +	   return completionHandler("\nNo stored Ad Click Attribution
data.\n"_s);

You probably want to put the first newline where this string is used.

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:593
> +	       NetworkSession* networkSession;

May as well initialize this to nullptr here, even though we know it will be
initialized later.  Ideally we would have C++17's if-with-initializer.	I sent
an email to webkit-dev.

> Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:366
> +    if (!shouldBlockCookies)

This seems like a good job for the operator ||

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:984
> +    initializeEphemeralStatelessCookielessSession(configuration);

We should probably lazily initialize this.  Hopefully most users will never
need to even allocate this.

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:1010
> +    configuration.allowsCellularAccess = YES;

This needs to come from NetworkSessionCreationParameters or from the other
sessions.  There's an internal client that needs to disallow cellular access
and it should disallow it for this, too.