Bug 63849 – WebKit2: Need callbacks for when a page goes in and out of the WebCore cache

• Component: WebKit2
• Status: ASSIGNED

[Jessie Berlin]

<rdar://problem/8108540>

[Jessie Berlin]

Created attachment 99536 [details]
Patch

[Andy Estes]

Comment on attachment 99536 [details]
Patch

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

> Source/WebCore/history/CachedFrame.cpp:249
> +    m_view->frame()->loader()->client()->didRemoveFromPageCache(m_document->docID());

It seems odd that the client gets a callback called DidRemoveFromPageCache before the actual work of removing the cached frame is completed.

[Jessie Berlin]

(In reply to comment #2)
> (From update of attachment 99536 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=99536&action=review
> 
> > Source/WebCore/history/CachedFrame.cpp:249
> > +    m_view->frame()->loader()->client()->didRemoveFromPageCache(m_document->docID());
> 
> It seems odd that the client gets a callback called DidRemoveFromPageCache before the actual work of removing the cached frame is completed.

Yes, I will move the call down after clear() has been called on the CachedFrame.

Originally, I was worried that would result in me using an already destroyed frame object. However, I have tested and also verified to myself that this is not the case.

Thanks for your feedback!

[Jessie Berlin]

Created attachment 99581 [details]
Patch (Take 2)

[Sam Weinig]

Comment on attachment 99581 [details]
Patch (Take 2)

I don't understand why sending the documentID is better than sending the Document itself.  That would be the usual way to do this.

[Jessie Berlin]

(In reply to comment #5)
> (From update of attachment 99581 [details])
> I don't understand why sending the documentID is better than sending the Document itself.  That would be the usual way to do this.

Should I create a new WK2 type for Document, just to have something to pass around (with no need for attributes or methods on it)?

[Sam Weinig]

(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 99581 [details] [details])
> > I don't understand why sending the documentID is better than sending the Document itself.  That would be the usual way to do this.
> 
> Should I create a new WK2 type for Document, just to have something to pass around (with no need for attributes or methods on it)?

Since a Document is a Node, you can use BundleNodeHandleRef.

[Jessie Berlin]

Created attachment 99734 [details]
Patch (using BundleNodeHandles instead of document ids)

[Gyuyoung Kim]

Comment on attachment 99734 [details]
Patch (using BundleNodeHandles instead of document ids)

Attachment 99734 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/8983724

[WebKit Review Bot]

Comment on attachment 99734 [details]
Patch (using BundleNodeHandles instead of document ids)

Attachment 99734 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8987425

[Early Warning System Bot]

Comment on attachment 99734 [details]
Patch (using BundleNodeHandles instead of document ids)

Attachment 99734 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8988431

[Collabora GTK+ EWS bot]

Comment on attachment 99734 [details]
Patch (using BundleNodeHandles instead of document ids)

Attachment 99734 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/8986504

[WebKit Review Bot]

Comment on attachment 99734 [details]
Patch (using BundleNodeHandles instead of document ids)

Attachment 99734 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/8983727

[Jessie Berlin]

Created attachment 99736 [details]
Patch (first build-fix attempt)

[Adam Roben (:aroben)]

Comment on attachment 99736 [details]
Patch (first build-fix attempt)

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

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePage.h:104
> +typedef void (*WKBundlePageDidSaveToPageCacheCallback)(WKBundlePageRef page, WKBundleFrameRef frame, WKBundleNodeHandleRef document, const void *clientInfo);
> +typedef void (*WKBundlePageDidRestoreFromPageCacheCallback)(WKBundlePageRef page, WKBundleFrameRef frame, WKBundleNodeHandleRef document, const void *clientInfo);
> +typedef void (*WKBundlePageDidRemoveFromPageCacheCallback)(WKBundlePageRef page, WKBundleFrameRef frame, WKBundleNodeHandleRef document, const void *clientInfo);

"PageCache" seems like the wrong term here. Based on the API, this looks like a document cache, not a page cache. I know "page cache" is the term we use throughout WebCore, but perhaps we should not expose that name as API?

[Sam Weinig]

(In reply to comment #15)
> (From update of attachment 99736 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=99736&action=review
> 
> > Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePage.h:104
> > +typedef void (*WKBundlePageDidSaveToPageCacheCallback)(WKBundlePageRef page, WKBundleFrameRef frame, WKBundleNodeHandleRef document, const void *clientInfo);
> > +typedef void (*WKBundlePageDidRestoreFromPageCacheCallback)(WKBundlePageRef page, WKBundleFrameRef frame, WKBundleNodeHandleRef document, const void *clientInfo);
> > +typedef void (*WKBundlePageDidRemoveFromPageCacheCallback)(WKBundlePageRef page, WKBundleFrameRef frame, WKBundleNodeHandleRef document, const void *clientInfo);
> 
> "PageCache" seems like the wrong term here. Based on the API, this looks like a document cache, not a page cache. I know "page cache" is the term we use throughout WebCore, but perhaps we should not expose that name as API?

For now, I think we should stick with PageCache as it is the familiar term.

[Adam Roben (:aroben)]

(In reply to comment #16)
> (In reply to comment #15)
> > (From update of attachment 99736 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=99736&action=review
> > 
> > > Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePage.h:104
> > > +typedef void (*WKBundlePageDidSaveToPageCacheCallback)(WKBundlePageRef page, WKBundleFrameRef frame, WKBundleNodeHandleRef document, const void *clientInfo);
> > > +typedef void (*WKBundlePageDidRestoreFromPageCacheCallback)(WKBundlePageRef page, WKBundleFrameRef frame, WKBundleNodeHandleRef document, const void *clientInfo);
> > > +typedef void (*WKBundlePageDidRemoveFromPageCacheCallback)(WKBundlePageRef page, WKBundleFrameRef frame, WKBundleNodeHandleRef document, const void *clientInfo);
> > 
> > "PageCache" seems like the wrong term here. Based on the API, this looks like a document cache, not a page cache. I know "page cache" is the term we use throughout WebCore, but perhaps we should not expose that name as API?
> 
> For now, I think we should stick with PageCache as it is the familiar term.

Familiar to whom? Have we ever exposed this concept in the WebKit API before?

[Adam Roben (:aroben)]

(In reply to comment #17)
> Have we ever exposed this concept in the WebKit API before?

I guess we have, in WebKit1.

[Adam Roben (:aroben)]

Still, what better time to give it a better name than when designing a new API from scratch?

[Brady Eidson]

(In reply to comment #15)
> (From update of attachment 99736 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=99736&action=review
> 
> > Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePage.h:104
> > +typedef void (*WKBundlePageDidSaveToPageCacheCallback)(WKBundlePageRef page, WKBundleFrameRef frame, WKBundleNodeHandleRef document, const void *clientInfo);
> > +typedef void (*WKBundlePageDidRestoreFromPageCacheCallback)(WKBundlePageRef page, WKBundleFrameRef frame, WKBundleNodeHandleRef document, const void *clientInfo);
> > +typedef void (*WKBundlePageDidRemoveFromPageCacheCallback)(WKBundlePageRef page, WKBundleFrameRef frame, WKBundleNodeHandleRef document, const void *clientInfo);
> 
> "PageCache" seems like the wrong term here. Based on the API, this looks like a document cache, not a page cache. I know "page cache" is the term we use throughout WebCore, but perhaps we should not expose that name as API?

The API could perhaps be "didSaveDocumentToPageCache", "didRestoreDocumentFromPageCache", and "didRemoveDocumentFromPageCache"

But it is *not* a document cache.  There is a lot more information saved/restored than just the Document.

Page cache is not only a precedented name, but is also somewhat accurate.  That's why we've finally gotten people to call it "page cache" instead of "back/forward cache" after all these years.  :)

[Brady Eidson]

(In reply to comment #19)
> Still, what better time to give it a better name than when designing a new API from scratch?

I'm not adverse to giving "page cache" a better name.

But "document cache" is not that name, because it is inaccurate.

[Sam Weinig]

(In reply to comment #21)
> (In reply to comment #19)
> > Still, what better time to give it a better name than when designing a new API from scratch?
> 
> I'm not adverse to giving "page cache" a better name.
> 
> But "document cache" is not that name, because it is inaccurate.

Maybe BackForwardCache?

[Brady Eidson]

(In reply to comment #22)
> (In reply to comment #21)
> > But "document cache" is not that name, because it is inaccurate.
> 
> Maybe BackForwardCache?

(In reply to comment #20)
> Page cache is not only a precedented name, but is also somewhat accurate.  That's why we've finally gotten people to call it "page cache" instead of "back/forward cache" after all these years.  :)

No.

[Jessie Berlin]

(In reply to comment #19)
> Still, what better time to give it a better name than when designing a new API from scratch?

After further discussion with Alexey and Brady, I am going to try to make this API more about document life-time.