[webkit-reviews] review granted: [Bug 174825] ResourceLoadStatistics grandfathering happens much too often : [Attachment 316397] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has granted Brady Eidson <beidson at apple.com>'s
request for review:
Bug 174825: ResourceLoadStatistics grandfathering happens much too often
https://bugs.webkit.org/show_bug.cgi?id=174825

Attachment 316397: Patch

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




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

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

Please see my comments on this iteration AND the previous one.

> Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStore.mm:478
> +   
store->scheduleClearInMemoryAndPersistent(std::chrono::system_clock::now() -
std::chrono::hours(hours),
WebKit::WebResourceLoadStatisticsStore::ShouldGrandfather::Yes);

ditto.

>> Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStore.mm:493
>> +- (void)_setResourceLoadStatisticsTestingCallback:(void
(^)(WKWebsiteDataStore *, NSString *))callback
> 
> Why doesn't the call site wait until it has set the callback to actually
enable the feature? It seems a bit odd to have an SPI that does 2 things,
especially considering we have an SPI to toggle the feature already.

Never mind this comment, I missed what
enableResourceLoadStatisticsAndSetTestingCallback() was doing.

> Source/WebKit/UIProcess/Storage/ResourceLoadStatisticsPersistentStorage.h:53
> +	   WriteLater,

I don't like this naming since we may write immediately even if you pass
WriteLater (i.e. if there has been no write in the last 5 minutes).
I suggest enum ShouldForceImmediateWrite { No, Yes }; or similar.

> Source/WebKit/UIProcess/Storage/ResourceLoadStatisticsPersistentStorage.h:55
> +    void scheduleOrWriteMemoryStore(WriteTime);

I think the default parameter value should be to not force an immediate write,
given that it is the common case. Then some call sites will look shorter.

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h:72
> +    void initialize();

I don't like having a separate initialize function.

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1098
> +    if (dataTypes.contains(WebsiteDataType::ResourceLoadStatistics) &&
m_resourceLoadStatistics) {

This looks duplicated. Can we move to a separate function?

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1321
> +	  
m_resourceLoadStatistics->setStatisticsTestingCallback(WTFMove(callback));

I think this could be passed to WebResourceLoadStatisticsStore::create(), with
the other lambda. Then we wouldn't need the initialize() function below.