[webkit-reviews] review granted: [Bug 174062] [WK2] Prevent ResourceLoadStatistics from triggering a cascade of read/write events : [Attachment 314613] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has granted Brent Fulgham
<bfulgham at webkit.org>'s request for review:
Bug 174062: [WK2] Prevent ResourceLoadStatistics from triggering a cascade of
read/write events
https://bugs.webkit.org/show_bug.cgi?id=174062

Attachment 314613: Patch

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




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

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

r=me with changes.

> Source/WebCore/platform/FileMonitor.h:53
> +#if USE(COCOA_EVENT_LOOP)

I wish we did not have to add an extra #if in this class' API. Do we really
need to expose this?

> Source/WebCore/platform/cocoa/FileMonitorCocoa.mm:70
>	   } else

Can we please ASSERT(flag & DISPATCH_VNODE_WRITE); in the else case?

> Source/WebKit2/ChangeLog:11
> +	   (1) The 'makeRefPtr' call in FileMonitor::startMonitoring was
capturing a reference to itself, preventing

As discussed offline, we should consider using a WeakPtr instead of a RefPtr to
address this issue in a follow-up.

> Source/WebKit2/ChangeLog:17
> +	   (2) 'syncWithExistingStatisticsStoageIfNeeded' was creating a
FileMonitor during the write operation,

typo: stoage.

> Source/WebKit2/ChangeLog:27
> +	   as a utility process, avoiding using the CPU when other work is
going on.

s/as a utility process/as utility QoS/

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:291
> +	   if (fileModificationTime <= m_lastStatisticsFileSyncTime) {

Can we move move these 2 lines to a utility method?
bool WebResourceLoadStatisticsStore::hasStatisticFileChangedSinceLastSync(path)
{
    return statisticsFileModificationTime(path) > m_lastStatisticsFileSyncTime;
}

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:292
> +	       // No need to grandfather in this case.n

typo, extra 'n'.

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:325
> +    WallTime fileModificationTime =
statisticsFileModificationTime(resourceLog);

Could use utility function.

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:336
> +    auto monitor = m_statisticsStorageMonitor ?
m_statisticsStorageMonitor->platformMonitor() : nullptr;

This means non-cocoa WK2 ports will no longer be able to enable logging at
compile time. I don't think we should expose the underlying monitor.

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:430
> +    auto monitor = m_statisticsStorageMonitor ?
m_statisticsStorageMonitor->platformMonitor() : nullptr;

Ditto about build breakage for other ports.

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:492
> +    // The FileMonitor captures itself, incrementing its refcount. Manually
stopping the monitor shuts down the lambda holding the extra

Should probably be a FIXME comment.