Bug 202660

Summary: Move SuspendedPage logic from WebProcessPool to new WebBackForwardCache class
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: WebKit2Assignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, beidson, commit-queue, ews-watchlist, ggaren, jiewen_tan, koivisto, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 202293, 202865    
Attachments:
Description Flags
WIP Patch
none
Patch
none
Patch
none
Patch none

Chris Dumez
Reported 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.
Attachments
WIP Patch (31.83 KB, patch)
2019-10-07 16:58 PDT, Chris Dumez 		no flags
DetailsFormatted DiffDiff
Patch (39.60 KB, patch)
2019-10-08 10:22 PDT, Chris Dumez 		no flags
DetailsFormatted DiffDiff
Patch (43.50 KB, patch)
2019-10-08 14:36 PDT, Chris Dumez 		no flags
DetailsFormatted DiffDiff
Patch (49.41 KB, patch)
2019-10-11 13:08 PDT, Chris Dumez 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Chris Dumez
Comment 1 2019-10-07 16:58:16 PDT
Created attachment 380372 [details] WIP Patch TestWebKitAPI.ProcessSwap.ReuseSuspendedProcessLoadHTMLString is failing. Still needs some cleanup.
Chris Dumez
Comment 2 2019-10-08 10:22:53 PDT
Created attachment 380440 [details] Patch
Chris Dumez
Comment 3 2019-10-08 14:36:55 PDT
Created attachment 380465 [details] Patch
Chris Dumez
Comment 4 2019-10-10 13:00:05 PDT
ping review?
Antti Koivisto
Comment 5 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?
Chris Dumez
Comment 6 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.
Chris Dumez
Comment 7 2019-10-11 13:08:03 PDT
Created attachment 380778 [details] Patch
Radar WebKit Bug Importer
Comment 8 2019-10-11 13:10:03 PDT
<rdar://problem/56200933>
Chris Dumez
Comment 9 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>
Chris Dumez
Comment 10 2019-10-11 13:40:56 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.