[webkit-reviews] review granted: [Bug 206438] Frequent sync BackForwardBackListCount/BackForwardForwardListCount IPC on reddit.com : [Attachment 388439] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 206438: Frequent sync BackForwardBackListCount/BackForwardForwardListCount
IPC on reddit.com
https://bugs.webkit.org/show_bug.cgi?id=206438

Attachment 388439: Patch

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




--- Comment #13 from Darin Adler <darin at apple.com> ---
Comment on attachment 388439
  --> https://bugs.webkit.org/attachment.cgi?id=388439
Patch

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

>> Source/WebKit/WebProcess/WebPage/WebBackForwardListProxy.cpp:63
>> +	clearCachedListCounts();
> 
> Technically, when the UIProcess restores the session, it could send the
counts along with the sessionState so we could populate here. Let me know if
you think we should do this.

Our past experience is that anything we can do to avoid sync calls ends up
helping us prevent surprise performance hiccups. So it seems we’d want to do
anything we could do to cut down on them. But despite the explanations you gave
earlier, still honestly not sure I understand why we can’t get rid of the sync
messaging entirely, which seems like the best longer term solution.

>> Source/WebKit/WebProcess/WebPage/WebBackForwardListProxy.cpp:102
>>     
m_page->send(Messages::WebPageProxy::BackForwardAddItem(toBackForwardListItemSt
ate(item.get())));
> 
> We could do a sendWithAsyncReply() here and get the updated counts back. Let
me know if you think I should do this.

Same thought.

> Source/WebKit/WebProcess/WebPage/WebBackForwardListProxy.cpp:113
> +    m_cachedBackForwardListCounts = WTFMove(backForwardListCounts);

No need to use WTFMove for a structure containing two integers.

> Source/WebKit/WebProcess/WebPage/WebBackForwardListProxy.cpp:148
> +	   m_cachedBackForwardListCounts = WTFMove(backForwardListCounts);

Ditto.