[webkit-reviews] review denied: [Bug 173800] Teach ResourceLoadStatistics to recognize changes in the file system : [Attachment 313847] Patch v5: Correct file deletion behavior.

[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 313847: Patch v5: Correct file deletion behavior.

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




--- Comment #17 from Sam Weinig <sam at webkit.org> ---
Comment on attachment 313847
  --> https://bugs.webkit.org/attachment.cgi?id=313847
Patch v5: Correct file deletion behavior.

View in context: https://bugs.webkit.org/attachment.cgi?id=313847&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.

As mentioned in my previous review:

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

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

I still don't understand why one platform would want locks and another would
not. Can you go into detail about why that is?

> Source/WebKit2/UIProcess/Cocoa/WebResourceLoadStatisticsStoreCocoa.mm:74
> +    m_statisticsStorageMonitor =
adoptOSObject(dispatch_source_create(DISPATCH_SOURCE_TYPE_VNODE, handle,
monitorMask, dispatch_get_main_queue()));
> +    
> +    dispatch_set_context(m_statisticsStorageMonitor.get(), this);
> +    

It looks like you didn't take my review feedback on creating a proper
abstraction for this.  Is there some objection you have to it?