[webkit-reviews] review denied: [Bug 173800] Teach ResourceLoadStatistics to recognize changes in the file system : [Attachment 313791] Patch

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has denied 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 313791: Patch

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




--- Comment #6 from Sam Weinig <sam at webkit.org> ---
Comment on attachment 313791
  --> https://bugs.webkit.org/attachment.cgi?id=313791
Patch

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

> Source/WebKit2/ChangeLog:10
> +	   Update the ResourceLoadStatistics logic to be aware that the
statistics data
> +	   file might change underneath it, and to take appropriate action when
it does.

Why is this being done?  What problem is it solving?

> Source/WebKit2/config.h:145
> +#if PLATFORM(COCOA)
> +#define USE_FILE_LOCK 1
> +#endif

This seems like something that should go into Platform.h

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:304
> +#if USE(FILE_LOCK)
> +    bool locked = lockFile(handle, WebCore::LockExclusive);
> +    ASSERT_UNUSED(locked, locked);
> +#endif

Is this code valid if you don't have file locks?

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.h:120
> +#if PLATFORM(COCOA)
> +    RetainPtr<FSEventStreamRef> m_statisticsStorageMonitoringStream;
> +#endif

Seems inappropriate to use a platform specific type here.  This should be
abstracted as we do with other platform specific concepts.