[webkit-reviews] review granted: [Bug 205844] The ITP database backend does not grandfather statistics correctly : [Attachment 387115] Patch

[bugzilla-daemon at webkit.org]

John Wilander <wilander at apple.com> has granted katherine_cheney at apple.com's
request for review:
Bug 205844: The ITP database backend does not grandfather statistics correctly
https://bugs.webkit.org/show_bug.cgi?id=205844

Attachment 387115: Patch

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




--- Comment #13 from John Wilander <wilander at apple.com> ---
Comment on attachment 387115
  --> https://bugs.webkit.org/attachment.cgi?id=387115
Patch

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

Overall, looks good to me. See inline comments before landing.

I looked up the existing code for avoiding grandfathering when all the website
data ITP cares about is deleted anyway.

It's in NetworkProcess::deleteWebsiteData() and looks like this:

#if ENABLE(RESOURCE_LOAD_STATISTICS)
    if (websiteDataTypes.contains(WebsiteDataType::ResourceLoadStatistics)) {
	if (auto* networkSession = this->networkSession(sessionID)) {
	    if (auto* resourceLoadStatistics =
networkSession->resourceLoadStatistics()) {
		auto deletedTypesRaw = websiteDataTypes.toRaw();
		auto monitoredTypesRaw =
WebResourceLoadStatisticsStore::monitoredDataTypes().toRaw();

		// If we are deleting all of the data types that the resource
load statistics store monitors
		// we do not need to re-grandfather old data.
		auto shouldGrandfather = ((monitoredTypesRaw & deletedTypesRaw)
== monitoredTypesRaw) ? ShouldGrandfatherStatistics::No :
ShouldGrandfatherStatistics::Yes;

	       
resourceLoadStatistics->scheduleClearInMemoryAndPersistent(modifiedSince,
shouldGrandfather, [clearTasksHandler = clearTasksHandler.copyRef()] { });
	    }
	}
    }
#endif

Putting it here for documentation.

> Source/WebKit/ChangeLog:3
> +	   The ITP database backend does not grandfather statistics correctly

I would rephrase this to say what the patch does rather than what the bug is.

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.h:1
38
> +    bool isNewResourceLoadStatisticsDatabaseFile() { return
m_isNewResourceLoadStatisticsDatabaseFile; }

This function can be const.

>
Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:239
> +		  
databaseStore.setIsNewResourceLoadStatisticsDatabaseFile(false);

Good. Important that we reset the state.

> Tools/ChangeLog:3
> +	   The ITP database backend does not grandfather statistics correctly

Same thing for the title.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/ResourceLoadStatistics.mm:115
> +	   // the memory store and testing grandfathering.

This comment was confusion to me. We're in a clause where the database is
enabled and the comments deals with the memory store note being enabled.

Can this be clarified somehow? I think there are similarly confusing comments
further down.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/ResourceLoadStatistics.mm:201
> +	   // the database store and testing grandfathering.

This comment is confusing since clearly we're in a clause where the database
store is not enabled. Could this be rephrased as "Since the database store …"?