Bug 202929

Summary:

[WK2] Have WebBackForwardCache class coordinate page caching in all WebProcesses

Product:

WebKit

Reporter:

Chris Dumez <cdumez>

Component:

Page Loading

Assignee:

Chris Dumez <cdumez>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

achristensen, beidson, commit-queue, dbates, ews-watchlist, ggaren, japhet, koivisto, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

See Also:

https://bugs.webkit.org/show_bug.cgi?id=202989

Bug Depends on:

Bug Blocks:

202293

Attachments:

Description	Flags
WIP Patch	none
WIP Patch	none
WIP Patch	none
WIP Patch	none
WIP Patch	none
WIP Patch	none
Patch	none
Patch	none
Patch	none

Chris Dumez

Reported 2019-10-14 08:31:26 PDT

Have WebBackForwardCache class coordinate page caching in all WebProcesses.

Attachments
WIP Patch (42.59 KB, patch) 2019-10-14 09:58 PDT, Chris Dumez	no flags	Details Formatted Diff Diff
WIP Patch (44.95 KB, patch) 2019-10-14 11:49 PDT, Chris Dumez	no flags	Details Formatted Diff Diff
WIP Patch (47.39 KB, patch) 2019-10-14 14:43 PDT, Chris Dumez	no flags	Details Formatted Diff Diff
WIP Patch (54.67 KB, patch) 2019-10-14 15:56 PDT, Chris Dumez	no flags	Details Formatted Diff Diff
WIP Patch (54.72 KB, patch) 2019-10-14 19:12 PDT, Chris Dumez	no flags	Details Formatted Diff Diff
WIP Patch (54.72 KB, patch) 2019-10-14 19:17 PDT, Chris Dumez	no flags	Details Formatted Diff Diff
Patch (68.88 KB, patch) 2019-10-14 20:18 PDT, Chris Dumez	no flags	Details Formatted Diff Diff
Patch (68.96 KB, patch) 2019-10-14 20:25 PDT, Chris Dumez	no flags	Details Formatted Diff Diff
Patch (68.97 KB, patch) 2019-10-14 21:00 PDT, Chris Dumez	no flags	Details Formatted Diff Diff
Show Obsolete (8) View All Add attachment proposed patch, testcase, etc.

Radar WebKit Bug Importer

Comment 1 2019-10-14 08:43:52 PDT

<rdar://problem/56250421>

Chris Dumez

Comment 2 2019-10-14 09:58:02 PDT

Created attachment 380889 [details] WIP Patch

Chris Dumez

Comment 3 2019-10-14 11:49:21 PDT

Created attachment 380905 [details] WIP Patch

Chris Dumez

Comment 4 2019-10-14 14:43:10 PDT

Created attachment 380921 [details] WIP Patch

Chris Dumez

Comment 5 2019-10-14 15:56:15 PDT

Created attachment 380928 [details] WIP Patch

Chris Dumez

Comment 6 2019-10-14 19:12:32 PDT

Created attachment 380947 [details] WIP Patch

Chris Dumez

Comment 7 2019-10-14 19:17:36 PDT

Created attachment 380949 [details] WIP Patch

Chris Dumez

Comment 8 2019-10-14 20:18:23 PDT

Created attachment 380950 [details] Patch

Chris Dumez

Comment 9 2019-10-14 20:25:42 PDT

Created attachment 380951 [details] Patch

Alex Christensen

Comment 10 2019-10-14 20:54:13 PDT

Comment on attachment 380951 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=380951&action=review > Source/WebKit/UIProcess/WebBackForwardCache.h:70 > + Vector<WebBackForwardListItem*> m_itemsWithCachedPage; Might it be useful to have inline capacity of 2 here? > Source/WebKit/UIProcess/WebProcessPool.h:598 > WebProcessProxy* m_prewarmedProcess { nullptr }; > WebProcessProxy* m_dummyProcessProxy { nullptr }; // A lightweight WebProcessProxy without backing process. These should probably be WeakPtr<WebProcessProxy>, maybe not in this patch.

Chris Dumez

Comment 11 2019-10-14 21:00:12 PDT

Created attachment 380956 [details] Patch

Chris Dumez

Comment 12 2019-10-14 21:00:46 PDT

(In reply to Alex Christensen from comment #10) > Comment on attachment 380951 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=380951&action=review > > > Source/WebKit/UIProcess/WebBackForwardCache.h:70 > > + Vector<WebBackForwardListItem*> m_itemsWithCachedPage; > > Might it be useful to have inline capacity of 2 here? Done. > > > Source/WebKit/UIProcess/WebProcessPool.h:598 > > WebProcessProxy* m_prewarmedProcess { nullptr }; > > WebProcessProxy* m_dummyProcessProxy { nullptr }; // A lightweight WebProcessProxy without backing process. > > These should probably be WeakPtr<WebProcessProxy>, maybe not in this patch. Why not. I can take care of this in a follow-up.

WebKit Commit Bot

Comment 13 2019-10-14 22:29:03 PDT

Comment on attachment 380956 [details] Patch Clearing flags on attachment: 380956 Committed r251121: <https://trac.webkit.org/changeset/251121>

WebKit Commit Bot

Comment 14 2019-10-14 22:29:05 PDT

All reviewed patches have been landed. Closing bug.

Antti Koivisto

Comment 15 2019-10-14 22:32:00 PDT

Comment on attachment 380956 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=380956&action=review > Source/WebKit/UIProcess/WebBackForwardCache.cpp:41 > +class EntryWithSuspendedPage final : public WebBackForwardCacheEntry { Is subclassing here really necessary? I feel the code would be easier to follow if WebBackForwardCacheEntry was a concrete type and having a suspended page or not was just a state. > Source/WebKit/UIProcess/WebProcessProxy.cpp:635 > + if (!!item->backForwardCacheEntry() != itemState.hasCachedPage) { That !! seems unnecessary

Chris Dumez

Comment 16 2019-10-15 07:53:23 PDT

(In reply to Antti Koivisto from comment #15) > Comment on attachment 380956 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=380956&action=review > > > Source/WebKit/UIProcess/WebBackForwardCache.cpp:41 > > +class EntryWithSuspendedPage final : public WebBackForwardCacheEntry { > > Is subclassing here really necessary? I feel the code would be easier to > follow if WebBackForwardCacheEntry was a concrete type and having a > suspended page or not was just a state. My earlier iteration wasn’t using inheritance and I found it less clear. However, given your feedback, I will give it another shot and upload a patch so you can decide. > > Source/WebKit/UIProcess/WebProcessProxy.cpp:635 > > + if (!!item->backForwardCacheEntry() != itemState.hasCachedPage) { > > That !! seems unnecessary Felt nicer to compare Booleans.

Note You need to log in before you can comment on or make changes to this bug.