[webkit-reviews] review denied: [Bug 182719] Make WebResourceLoadStatisticsStore::processStatisticsAndDataRecords() call WebProcessProxy::notifyPageStatisticsAndDataRecordsProcessed() in a proper callback : [Attachment 333988] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Feb 16 08:53:37 PST 2018


Brent Fulgham <bfulgham at webkit.org> has denied John Wilander
<wilander at apple.com>'s request for review:
Bug 182719: Make
WebResourceLoadStatisticsStore::processStatisticsAndDataRecords() call
WebProcessProxy::notifyPageStatisticsAndDataRecordsProcessed() in a proper
callback
https://bugs.webkit.org/show_bug.cgi?id=182719

Attachment 333988: Patch

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




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

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

I think this is close, but not quite right. r- to move the last few steps of
the method inside the completion handlers.

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:282
> +    pruneStatisticsIfNeeded();

Shouldn't "pruneStatisticsIfNeeded()" and
"m_persistentStorage.scheduleOrWriteMemoryStore" be part of the completion
handler?

I think part of the flakiness is that 'removeDataRecords' can take time to
complete (hopping from work queue, to main thread, to work queue, to main
thread). I think after this patch, removeDataRecords will be called at the
expected time now, but that 'pruneStatisticsIfNeeded' might run before
removeDataRecords completes (since removeDataRecords hops to the main thread
before doing the delete, which might allow the work queue to return and call
'pruneStatisticsIfNeeded' before the deletes happen.

Likewise, I don't think we want to schedule the write of the in-memory store
until these bits of work are finished, so they should be inside the completion
handler, too.


More information about the webkit-reviews mailing list