[webkit-reviews] review denied: [Bug 203623] Resource Load Statistics: Flush the shared ResourceLoadObserver when the webpage is closed by JavaScript : [Attachment 382363] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has denied John Wilander <wilander at apple.com>'s
request for review:
Bug 203623: Resource Load Statistics: Flush the shared ResourceLoadObserver
when the webpage is closed by JavaScript
https://bugs.webkit.org/show_bug.cgi?id=203623

Attachment 382363: Patch

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




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

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

> Source/WebCore/loader/ResourceLoadObserver.cpp:39
> +    deleteShared();

I don't think we need this deleteShared() method at all. We could simply
inline:
delete sharedObserver();

delete does the null check for you and you don't need to set to null since the
next line will do it for you.

> Source/WebCore/loader/ResourceLoadObserver.cpp:50
> +ResourceLoadObserver*& ResourceLoadObserver::sharedIfExists()

We should not be adding an ampersand here, this is a public method.

> Source/WebCore/loader/ResourceLoadObserver.cpp:55
> +void ResourceLoadObserver::deleteShared()

We can delete this method altogether as I mentioned above.

> Source/WebCore/loader/ResourceLoadObserver.cpp:57
> +    if (auto*& observer = sharedIfExists()) {

This should be using sharedObserver(), not sharedIfExists().

> Source/WebCore/loader/ResourceLoadObserver.h:42
> +    WEBCORE_EXPORT static ResourceLoadObserver*& sharedIfExists();

No ampersand.

> Source/WebCore/loader/ResourceLoadObserver.h:64
> +    static void deleteShared();

Not needed.

> Source/WebCore/page/DOMWindow.cpp:1042
> +    ResourceLoadObserver::shared().updateCentralStatisticsStore();

Maybe we do this way below, only if close() does not return early?

>
Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:286
> +	   m_statisticsStore->updateCookieBlocking([this]() {

What guarantees that |this| stays alive here. Looks to me like you need to
protect/ref it.

> Source/WebKit/UIProcess/WebProcessPool.cpp:2351
> +    for (auto* processPool : WebProcessPool::allProcessPools()) {

This is a method of a given WebProcessPool (e.g. not static). So why would it
iterate over all process pools?

> Source/WebKit/UIProcess/WebProcessPool.cpp:2353
> +	       if (process->canSendMessage())

This check is not needed

> Source/WebKit/WebProcess/WebProcess.cpp:674
> +#if ENABLE(RESOURCE_LOAD_STATISTICS)

Could we move the #if ENABLE(RESOURCE_LOAD_STATISTICS) inside the
implementation of flushResourceLoadStatistics to make the call sites look
better?

> Source/WebKit/WebProcess/WebProcess.cpp:1568
> +	   WebCore::ResourceLoadObserver::setShared(new
WebResourceLoadObserver);

This could be on one line:
WebCore::ResourceLoadObserver::setShared(enabled ? new WebResourceLoadObserver
: nullptr);

Also, are we sure that setResourceLoadStatisticsEnabled(true) cannot be called
twice in a raw? In such cases we would destroy the observer unnecessarily and
possibly lose data.

On the same note, should the WebResourceLoadObserver destructor flush? So that
if you have pending data then disable ITP in this process, then the pending
statistics are not lost and still sent to the network process?