Bug 202660 – Move SuspendedPage logic from WebProcessPool to new WebBackForwardCache class

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Chris Dumez]

Move SuspendedPage logic from WebProcessPool to new WebBackForwardCache class. This is a step towards implementing back / forward cache handling in the UIProcess.

[Chris Dumez]

Created attachment 380372 [details]
WIP Patch

TestWebKitAPI.ProcessSwap.ReuseSuspendedProcessLoadHTMLString is failing. Still needs some cleanup.

[Chris Dumez]

Created attachment 380440 [details]
Patch

[Chris Dumez]

Created attachment 380465 [details]
Patch

[Chris Dumez]

ping review?

[Antti Koivisto]

Comment on attachment 380465 [details]
Patch

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

> Source/WebKit/UIProcess/SuspendedPageProxy.cpp:147
> +void SuspendedPageProxy::setBackForwardListItem(WebBackForwardListItem* item)
> +{
> +    ASSERT(!m_backForwardListItem || !item);

Considering the assert it would be better to divide this into separate set/clear functions (where set takes a reference).

> Source/WebKit/UIProcess/SuspendedPageProxy.cpp:149
> +    if (m_backForwardListItem)
> +        process().processPool().backForwardCache().unregisterItemWithCachedPage(*m_backForwardListItem);

The only call sites for setBackForwardListItem are in WebBackForwardListItem. Is it odd that registering WebBackForwardListItems is managed here instead of the call sites?

[Chris Dumez]

(In reply to Antti Koivisto from comment #5)
> Comment on attachment 380465 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=380465&action=review
> 
> > Source/WebKit/UIProcess/SuspendedPageProxy.cpp:147
> > +void SuspendedPageProxy::setBackForwardListItem(WebBackForwardListItem* item)
> > +{
> > +    ASSERT(!m_backForwardListItem || !item);
> 
> Considering the assert it would be better to divide this into separate
> set/clear functions (where set takes a reference).
> 
> > Source/WebKit/UIProcess/SuspendedPageProxy.cpp:149
> > +    if (m_backForwardListItem)
> > +        process().processPool().backForwardCache().unregisterItemWithCachedPage(*m_backForwardListItem);
> 
> The only call sites for setBackForwardListItem are in
> WebBackForwardListItem. Is it odd that registering WebBackForwardListItems
> is managed here instead of the call sites?

Thanks Antti, very useful feedback. I'll make the suggested improvements before landing.

[Chris Dumez]

Created attachment 380778 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/56200933>

[Chris Dumez]

Comment on attachment 380778 [details]
Patch

Clearing flags on attachment: 380778

Committed r251022: <https://trac.webkit.org/changeset/251022>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.