[webkit-reviews] review denied: [Bug 195420] Enable LayoutTests using ResourceLoadStatistics SQLite backend : [Attachment 378891] 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 378891: Patch

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




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

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

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:458
> +    auto registrableDomainID =
domainID(loadStatistics.registrableDomain).value();

Why this change? The code looked a bit safer before? However, it is
unconditionally de-refing an optional.

>
Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:162
> +	       FileSystem::makeAllDirectories(resourceLoadStatisticsDirectory);

It feels like this should be in the ResourceLoadStatisticsDatabaseStore, right
before opening.

> Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.h:192
> +    void flushAndDestroyPersistentStore();

Why is this public now?

> Source/WebKit/NetworkProcess/NetworkSession.cpp:166
> +    m_resourceLoadStatistics->flushAndDestroyPersistentStore();

This seems wrong, you should be calling destroyResourceLoadStatistics()
instead.

> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:270
> +void WKWebsiteDataStoreSetUseITPDatabase(WKWebsiteDataStoreRef dataStoreRef,
bool value)

Something is odd here, why use a completion handler internally but not expose
it on your SPI. Either you need a completion handler (and you should expose it
here), or you don't and the implementation should stop passing a completion
handler around.

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:59
> +

Unnecessary change.