[webkit-reviews] review granted: [Bug 174435] Moved filesystem code out of WebResourceLoadStatisticsStore class : [Attachment 315349] Patch

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has granted Chris Dumez
<cdumez at apple.com>'s request for review:
Bug 174435: Moved filesystem code out of WebResourceLoadStatisticsStore class
https://bugs.webkit.org/show_bug.cgi?id=174435

Attachment 315349: Patch

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




--- Comment #14 from Brent Fulgham <bfulgham at webkit.org> ---
Comment on attachment 315349
  --> https://bugs.webkit.org/attachment.cgi?id=315349
Patch

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

>>>
Source/WebKit2/UIProcess/Storage/ResourceLoadStatisticsPersistentStorage.cpp:20
9
>>> +
>> 
>> In the original implementation I made a call to sync the memory store with
the on-disk store here. I still think this is necessary, even though we monitor
file change/file delete events because we stop monitoring file events after
delete (since the we can't make a file handle to a non-existent file).
>> 
>> A second process could write a new copy of the file while we are running,
which means we could lose data. A potentially cleaner approach might be to
monitor the directory holding the statistics file, which would tell us when a
new files was created.
>> 
>> I took the easier approach of checking for data before writing, and syncing
before conducting the write.
>> 
>> I think this change loses the ability to react to new data appearing on disk
while we are not monitoring the disk.
>> 
>> I guess we could fix this in a separate patch (adding a new handler for
directory changes).
> 
> I wanted to discuss the few minor behavior changes with you today. Yes, there
was a call to sync the memory cache before writing (using
syncWithExistingStatisticsStorageIfNeeded) and I dropped it. The reason I
dropped it it that it seemed to be a no-op. If we're in this function, then it
means we have in memory data to write.
syncWithExistingStatisticsStorageIfNeeded() was calling refreshFromDisk(),
which would call populateFromDecoder(). populateFromDecoder() did an early
return if we had any data. Even if populateFromDecoder() did not do an early
return, then we'd still be in trouble I believe because this "sync" before
writing would override our new data.
> 
> I understand now why the existing code was in place, however, unless I am
missing something, I don't think it did what it was supposed to do.

Sounds good. I propose creating a separate issue to do add a directory monitor
when we detect a file delete, which will go away when we see the file come back
(or we write a file). Then we can handle the sync of existing data when new
data is written, not at the moment we are about to write.