[webkit-reviews] review denied: [Bug 195420] Enable LayoutTests using ResourceLoadStatistics SQLite backend : [Attachment 378649] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has denied Katherine_cheney at apple.com's request
for review:
Bug 195420: Enable LayoutTests using ResourceLoadStatistics SQLite backend
https://bugs.webkit.org/show_bug.cgi?id=195420

Attachment 378649: Patch

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




--- Comment #8 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 378649
  --> https://bugs.webkit.org/attachment.cgi?id=378649
Patch

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

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:862
> +    for (auto& regDomain : domains)

No abbreviations please. regDomain == :(

>
Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:158
> +   
initializeStatisticsStore(RuntimeEnabledFeatures::sharedFeatures().isITPDatabas
eEnabled(), resourceLoadStatisticsDirectory.isolatedCopy(),
shouldIncludeLocalhost);

No .isolatedCopy() here, see comment below.

Also, this is the first time I see RuntimeEnabledFeatures::sharedFeatures()
used in the network process, this is usually for WebProcesses.

>
Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:172
> +    postTask([this, databaseEnabled = databaseEnabled,
resourceLoadStatisticsDirectory = resourceLoadStatisticsDirectory,
shouldIncludeLocalhost] {

This is not thread-safe:
resourceLoadStatisticsDirectory =
resourceLoadStatisticsDirectory.isolatedCopy()
Also, no need for "= databaseEnabled", please omit.

> Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.h:193
> +    void initializeStatisticsStore(bool, const String&
resourceLoadStatisticsDirectory, ShouldIncludeLocalhost);

Can't we re-construct the whole WebResourceLoadStatisticsStore instead of
making such a method public?