<rdar://problem/8108540>
Created attachment 99536 [details] Patch
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.
(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!
Created attachment 99581 [details] Patch (Take 2)
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.
(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)?
(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.
Created attachment 99734 [details] Patch (using BundleNodeHandles instead of document ids)
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
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
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
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
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
Created attachment 99736 [details] Patch (first build-fix attempt)
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?
(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.
(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?
(In reply to comment #17) > Have we ever exposed this concept in the WebKit API before? I guess we have, in WebKit1.
Still, what better time to give it a better name than when designing a new API from scratch?
(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. :)
(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.
(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?
(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.
(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.