[webkit-reviews] review denied: [Bug 195420] Enable LayoutTests using ResourceLoadStatistics SQLite backend : [Attachment 378649] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Sep 12 09:52:19 PDT 2019
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?
More information about the webkit-reviews
mailing list