Bug 202660 - Move SuspendedPage logic from WebProcessPool to new WebBackForwardCache class
Summary: Move SuspendedPage logic from WebProcessPool to new WebBackForwardCache class
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks: 202293 202865
  Show dependency treegraph
 
Reported: 2019-10-07 16:55 PDT by Chris Dumez
Modified: 2019-10-11 14:01 PDT (History)
8 users (show)

See Also:


Attachments
WIP Patch (31.83 KB, patch)
2019-10-07 16:58 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (39.60 KB, patch)
2019-10-08 10:22 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (43.50 KB, patch)
2019-10-08 14:36 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (49.41 KB, patch)
2019-10-11 13:08 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2019-10-07 16:55:07 PDT
Move SuspendedPage logic from WebProcessPool to new WebBackForwardCache class. This is a step towards implementing back / forward cache handling in the UIProcess.
Comment 1 Chris Dumez 2019-10-07 16:58:16 PDT
Created attachment 380372 [details]
WIP Patch

TestWebKitAPI.ProcessSwap.ReuseSuspendedProcessLoadHTMLString is failing. Still needs some cleanup.
Comment 2 Chris Dumez 2019-10-08 10:22:53 PDT
Created attachment 380440 [details]
Patch
Comment 3 Chris Dumez 2019-10-08 14:36:55 PDT
Created attachment 380465 [details]
Patch
Comment 4 Chris Dumez 2019-10-10 13:00:05 PDT
ping review?
Comment 5 Antti Koivisto 2019-10-10 14:15:41 PDT
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?
Comment 6 Chris Dumez 2019-10-11 08:27:14 PDT
(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.
Comment 7 Chris Dumez 2019-10-11 13:08:03 PDT
Created attachment 380778 [details]
Patch
Comment 8 Radar WebKit Bug Importer 2019-10-11 13:10:03 PDT
<rdar://problem/56200933>
Comment 9 Chris Dumez 2019-10-11 13:40:54 PDT
Comment on attachment 380778 [details]
Patch

Clearing flags on attachment: 380778

Committed r251022: <https://trac.webkit.org/changeset/251022>
Comment 10 Chris Dumez 2019-10-11 13:40:56 PDT
All reviewed patches have been landed.  Closing bug.