[webkit-reviews] review denied: [Bug 203017] Resource Load Statistics (experimental): Block all third-party cookies on websites without prior user interaction : [Attachment 381046] 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 203017: Resource Load Statistics (experimental): Block all third-party
cookies on websites without prior user interaction
https://bugs.webkit.org/show_bug.cgi?id=203017

Attachment 381046: Patch

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




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

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

Good for the most part.

> Source/WebCore/platform/network/NetworkStorageSession.cpp:144
> +    m_registrableDomainsWithUserInteractionAsFirstParty.clear();
> +    m_registrableDomainsWithUserInteractionAsFirstParty.add(domains.begin(),
domains.end());

Would it not be more efficient to only add the newly-interacted-with domains
instead of clearing and re-adding all of the interacted-with domains?

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:1761
> +    SQLiteStatement statement(m_database, "SELECT registrableDomain FROM
ObservedDomains WHERE hadUserInteraction = 1"_s);
> +    if (statement.prepare() != SQLITE_OK)

Probably for a different patch:
We probably don't want to prepare statements every time we call these
functions.  We could cache prepared statements

> Source/WebKit/NetworkProcess/NetworkSession.cpp:117
> +    m_adClickAttribution->setIsDebugModeEnabledFunction([this, weakThis =
makeWeakPtr(this)] () {

This isn't a good pattern to construct an object then call a bunch of setters
for callbacks.	I think it would be cleaner to have an AdClickAttributionClient
interface that the NetworkSession implements, then pass it in as a constructor
parameter.

> Source/WebKit/Shared/WebPreferences.yaml:1771
> +  humanReadableName: "Block 3p Cookies On Sites Without Interaction (ITP)"

I don't think "3p" is human readable.

> Tools/WebKitTestRunner/TestInvocation.cpp:1838
> +   
WKPagePostMessageToInjectedBundle(TestController::singleton().mainWebView()->pa
ge(), messageName.get(), 0);

nullptr