[webkit-reviews] review denied: [Bug 188109] Resource Load Statistics: Remove partitioned cookies for reduced complexity, lower memory footprint, and ability to support more platforms : [Attachment 346128] Patch

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has denied Alex Christensen
<achristensen at apple.com>'s request for review:
Bug 188109: Resource Load Statistics: Remove partitioned cookies for reduced
complexity, lower memory footprint, and ability to support more platforms
https://bugs.webkit.org/show_bug.cgi?id=188109

Attachment 346128: Patch

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




--- Comment #20 from Brent Fulgham <bfulgham at webkit.org> ---
Comment on attachment 346128
  --> https://bugs.webkit.org/attachment.cgi?id=346128
Patch

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

400KB patches are an anti-pattern! However, this looks reasonable. You
improperly deleted some logging routines that we still need, so r- to correct
those cases. Otherwise this looks pretty close to ready. I'll look at the test
failures next and see if there are any other comments.

> Source/WebCore/ChangeLog:29
> +	   reflect the code changes and to shorten their names meanwhile (as 

I don't think you need the word "meanwhile" here.

> Source/WebCore/ChangeLog:46
> +	       the Storage Access API. This was previously handled by

handled by what? The suspense is killing me! :-)

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:-1011
> -static void logCookieInformationInternal(const String& label, const void*
loggedObject, const WebCore::NetworkStorageSession& networkStorageSession,
const WebCore::URL& partition, const WebCore::SameSiteInfo& sameSiteInfo, const
WebCore::URL& url, const String& referrer, std::optional<uint64_t> frameID,
std::optional<uint64_t> pageID, std::optional<uint64_t> identifier)

I don't think you want to get rid of this logging. You should just remove the
partitioning aspect of this. We still would like to log origins with storage
access, etc., when running in debug/diagnostic mode.

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:-1080
> -	   logCookieInformationInternal(label, loggedObject,
networkStorageSession, partition, sameSiteInfo, url, referrer, frameID, pageID,
identifier);

You should keep this, just not the partition part of the logging.

> Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:-404
> -	   LOG(NetworkSession, "%llu %s cookies for redirect URL %s", [m_task
taskIdentifier], (requiredStoragePartition.isEmpty() ? "Not partitioning" :
"Partitioning"), request.url().string().utf8().data());

You need to keep this logging, just remove the portioning data from the log
entry.

> Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:-406
> -	   applyCookiePartitioningPolicy(requiredStoragePartition,
m_task.get()._storagePartitionIdentifier);

But of course this line can go away.

> Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp:938
>  bool
ResourceLoadStatisticsMemoryStore::wasAccessedAsFirstPartyDueToUserInteraction(
const ResourceLoadStatistics& current, const ResourceLoadStatistics& updated)
const

I wonder if we even need this method anymore, if there is no partitioning?