[webkit-reviews] review granted: [Bug 187055] Split memory store logic out of WebResourceLoadStatisticsStore to clarify threading model : [Attachment 343717] 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 187055: Split memory store logic out of WebResourceLoadStatisticsStore to
clarify threading model
https://bugs.webkit.org/show_bug.cgi?id=187055

Attachment 343717: Patch

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




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

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

Looks good, and the tests all pass. Nice work on this huge refactoring job!
r=me.

> Source/WebKit/ChangeLog:4
> +	   https://bugs.webkit.org/show_bug.cgi?id=187055

Radar please!

> Source/WebKit/ChangeLog:14
> +	   objects merely proxies calls from WebKit to those persistent /
memory stores and takes care of hoping back and

"... takes care of HOPPING back and"

> Source/WebKit/ChangeLog:18
> +	   I fixed those in this patch now that the model is clearer.

Awesome!

> Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp:468
> +    RunLoop::main().dispatch([subFramePrimaryDomain =
subFramePrimaryDomain.isolatedCopy(), topFramePrimaryDomain =
topFramePrimaryDomain.isolatedCopy(), frameID, pageID, store =
makeRef(m_store), callback = WTFMove(callback)]() mutable {

I guess this is one of the cases where we were calling on the wrong thread in
the original code!

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:129
> +	   m_memoryStore->setPersistentStorage(*m_persistentStorage);

I wonder if the m_persistentStore constructor could establish this
relationship?

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:183
> +// On background queue due to IPC.

Thank you for this comment! I thought this was a mistake at first! :-)