[webkit-reviews] review granted: [Bug 173800] Teach ResourceLoadStatistics to recognize changes in the file system : [Attachment 314054] 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 173800: Teach ResourceLoadStatistics to recognize changes in the file
system
https://bugs.webkit.org/show_bug.cgi?id=173800

Attachment 314054: Patch

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




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

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

r=me with a few comments.

> Source/WTF/wtf/DispatchPtr.h:74
> +	   std::exchange(m_ptr, nullptr);

What's the point of std::exchange since you do not use the return value? Seems
like this should be:
auto ptr = std::exchange(m_ptr, nullptr);
if (ptr)
    dispatch_release(ptr);

> Source/WebCore/platform/FileMonitor.h:29
> +#include <wtf/Noncopyable.h>

Is this really needed?

> Source/WebCore/platform/cocoa/FileMonitorCocoa.mm:57
> +    RefPtr<FileMonitor> protectedThis(this);

Not needed if comment below is applied.

> Source/WebCore/platform/cocoa/FileMonitorCocoa.mm:58
> +    dispatch_source_set_event_handler(m_platformMonitor.get(), [this,
protectedThis, fileMonitor = m_platformMonitor.get()] () mutable {

protectedThis = makeRefPtr(this) (I am assuming makeRef(*this) does not work
because this expects an ObjC block)

> Source/WebCore/platform/cocoa/FileMonitorCocoa.mm:59
> +	   // If this is getting called after the monitor was cancelled, just
drop the notification.

Could we add an assertion here to make sure we are not on the main thread?

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:193
> +	   m_statisticsStorageMonitor = nullptr;

I do not think we should call the FileMonitor destructor on the main thread.
Since it is constructed and used solely on the statisticsQueue, I think we
should destroy it on the statisticsQueue as well. E.g.
m_statisticsQueue->dispatch([statisticsStorageMonitor =
WTFMove(m_statisticsStorageMonitor)] { });

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:414
> +	   if (type == FileMonitor::FileChangeType::Modification)

Can we please use a switch() (without default: statement) instead of an if?

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:417
> +	       clearInMemoryData();

Once a statistics file is deleted by someone else, we clear in memory data but
do not null out m_statisticsStorageMonitor. Is that OK?
If we want to start monitoring again at some point,
startMonitoringStatisticsStorage() will be a no-op because
m_statisticsStorageMonitor is not null.

> Tools/TestWebKitAPI/Tests/WebCore/FileMonitor.cpp:105
> +    command.append("echo \"");

appendLiteral()

> Tools/TestWebKitAPI/Tests/WebCore/FileMonitor.cpp:107
> +    command.append("\" > ");

appendLiteral()

> Tools/TestWebKitAPI/Tests/WebCore/FileMonitor.cpp:152
> +	   if (type == FileMonitor::FileChangeType::Modification)

switch()

> Tools/TestWebKitAPI/Tests/WebCore/FileMonitor.cpp:190
> +	   if (type == FileMonitor::FileChangeType::Modification)

switch()

> Tools/TestWebKitAPI/Tests/WebCore/FileMonitor.cpp:246
> +	   if (type == FileMonitor::FileChangeType::Modification)

switch()

> Tools/TestWebKitAPI/Tests/WebCore/FileMonitor.cpp:255
> +	   command.append("rm -f ");

appendLiteral()