[webkit-reviews] review granted: [Bug 193556] Implement message handlers for NetworkProcess-based ResourceLoadStatistics : [Attachment 359548] Patch

[bugzilla-daemon at webkit.org]

Alex Christensen <achristensen at apple.com> has granted Brent Fulgham
<bfulgham at webkit.org>'s request for review:
Bug 193556: Implement message handlers for NetworkProcess-based
ResourceLoadStatistics
https://bugs.webkit.org/show_bug.cgi?id=193556

Attachment 359548: Patch

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




--- Comment #18 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 359548
  --> https://bugs.webkit.org/attachment.cgi?id=359548
Patch

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

r=me.  Good luck landing this.

> Source/WebCore/loader/ResourceLoadObserver.cpp:72
> +void
ResourceLoadObserver::setLogUserInteractionNotificationCallback(WTF::Function<v
oid(PAL::SessionID, const String&)>&& callback)

WTF:: is unnecessary.

>
Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:107
> +	   postTaskReply([completionHandler = WTFMove(completionHandler)] ()
mutable {
> +	       completionHandler();
> +	   });

postTaskReply(WTFMove(completionHandler));

>
Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:291
> -	   m_memoryStore->hasStorageAccess(subFramePrimaryDomain,
topFramePrimaryDomain, frameID, pageID, [completionHandler =
WTFMove(completionHandler)](bool hasStorageAccess) mutable {
> +	   m_memoryStore->hasStorageAccess(subFramePrimaryDomain,
topFramePrimaryDomain, frameID.value(), pageID, [completionHandler =
WTFMove(completionHandler)](bool hasStorageAccess) mutable {

This .value() shouldn't be necessary.  Just make the callee take an Optional.

>
Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:1195
> +    if (m_websiteDataStore)
> +	  
WebProcessProxy::topPrivatelyControlledDomainsWithWebsiteData(dataTypes,
shouldNotifyPage, WTFMove(completionHandler));
> +
> +    if (m_networkSession)
> +	  
m_networkSession->topPrivatelyControlledDomainsWithWebsiteData(dataTypes,
shouldNotifyPage, WTFMove(completionHandler));

Maybe we should early return if there's a websiteDataStore, and call the
completion handler if there are neither websiteDataStore nor networkSession
just to make sure it's even more impossible to misuse the completion handler. 
There are a few such things in this file.

> Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.h:65
> +enum class ShouldGrandfather {

This no longer has the scope of WebResourceLoadStatisticsStore, so it could use
a better name.	And it can use a bool for storage and be on one line.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:-653
> -	       ASSERT(RunLoop::isMain());

Why are we removing this assertion?

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1370
> +    // FIXME: No API to delete credentials by origin

Should this have the same bugzilla number as the other FIXMEs in this patch?

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:250
> +    while (!m_allStorageAccessEntriesCallbackMap.isEmpty())
> +	  
m_allStorageAccessEntriesCallbackMap.take(m_allStorageAccessEntriesCallbackMap.
begin()->key)({ });

Using sendWithAsyncReply would make all these autogenerated and reduce the
boilerplate code in this patch significantly.