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

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Chris Dumez]

Have WebBackForwardCache class coordinate page caching in all WebProcesses.

[Radar WebKit Bug Importer]

<rdar://problem/56250421>

[Chris Dumez]

Created attachment 380889 [details]
WIP Patch

[Chris Dumez]

Created attachment 380905 [details]
WIP Patch

[Chris Dumez]

Created attachment 380921 [details]
WIP Patch

[Chris Dumez]

Created attachment 380928 [details]
WIP Patch

[Chris Dumez]

Created attachment 380947 [details]
WIP Patch

[Chris Dumez]

Created attachment 380949 [details]
WIP Patch

[Chris Dumez]

Created attachment 380950 [details]
Patch

[Chris Dumez]

Created attachment 380951 [details]
Patch

[Alex Christensen]

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]

Created attachment 380956 [details]
Patch

[Chris Dumez]

(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 on attachment 380956 [details]
Patch

Clearing flags on attachment: 380956

Committed r251121: <https://trac.webkit.org/changeset/251121>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Antti Koivisto]

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]

(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.