Bug 193349

Created attachment 358891 [details] Patch

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

Ouch, I forgot to add the new function to “webkit2gtk-4.0-sections.txt”, I'll upload an updated patch in a moment.

(In reply to Adrian Perez from comment #3) > Ouch, I forgot to add the new function to “webkit2gtk-4.0-sections.txt”, > I'll upload an updated patch in a moment. …and to “wpe-0.1-sections.txt” as well.

Created attachment 358893 [details] Patch v2 Now with the gtk-doc bits added

Comment on attachment 358893 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=358893&action=review > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:4153 > + * Note that the currently loaded page is *not* changed, and in order to clear the contents > + * being displayed webkit_web_view_load_uri() should be used to load the `about:blank` > + * resource. We should also say here that the bf list won't be empty, it will have a single item for the currently loaded page. > Source/WebKit/UIProcess/WebPageProxy.h:811 > + void clearSessionState() { backForwardClear(); } There's more in the session state than the bf list, no? > Tools/TestWebKitAPI/Tests/WebKitGLib/TestBackForwardList.cpp:335 > + g_assert_cmpuint(webkit_back_forward_list_get_length(bfList), ==, 1); Should we also check that the item is indeed he currently loaded page with BackForwardListTest::checkItem? > Tools/TestWebKitAPI/Tests/WebKitGLib/TestBackForwardList.cpp:337 > + g_assert(!webkit_web_view_can_go_back(view.get())); > + g_assert(!webkit_web_view_can_go_forward(view.get())); Use g_assert_false

(In reply to Carlos Garcia Campos from comment #6) > Comment on attachment 358893 [details] > Patch v2 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=358893&action=review > > > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:4153 > > + * Note that the currently loaded page is *not* changed, and in order to clear the contents > > + * being displayed webkit_web_view_load_uri() should be used to load the `about:blank` > > + * resource. > > We should also say here that the bf list won't be empty, it will have a > single item for the currently loaded page. Good point, will do. > > Source/WebKit/UIProcess/WebPageProxy.h:811 > > + void clearSessionState() { backForwardClear(); } > > There's more in the session state than the bf list, no? I checked what WebPageProxy::restoreFromSessionState() does: the only other things it restores are “m_renderTreeSize” which I do not have a super clear idea what it is used for (they appear to be used by the iOS+Mac ViewGestureController and ViewSnapshotStore implementations), but as far as I can understand it does not need changing because the currently loaded page is kept. The rest of things which need updating are handled properly because the WebBackForwardList::clear() method itself does calls to the didChangeBackForwardList() method to make the Page (and possible other objects) notice the changes. > > Tools/TestWebKitAPI/Tests/WebKitGLib/TestBackForwardList.cpp:335 > > + g_assert_cmpuint(webkit_back_forward_list_get_length(bfList), ==, 1); > > Should we also check that the item is indeed he currently loaded page with > BackForwardListTest::checkItem? It won't hurt to have this check as well, so I will add it. > > Tools/TestWebKitAPI/Tests/WebKitGLib/TestBackForwardList.cpp:337 > > + g_assert(!webkit_web_view_can_go_back(view.get())); > > + g_assert(!webkit_web_view_can_go_forward(view.get())); > > Use g_assert_false Okay.

(In reply to Adrian Perez from comment #7) > (In reply to Carlos Garcia Campos from comment #6) > > Comment on attachment 358893 [details] > > Patch v2 > > > > [...] > > > > > Source/WebKit/UIProcess/WebPageProxy.h:811 > > > + void clearSessionState() { backForwardClear(); } > > > > There's more in the session state than the bf list, no? > > I checked what WebPageProxy::restoreFromSessionState() does: the only > other things it restores are “m_renderTreeSize” which I do not have a > super clear idea what it is used for (they appear to be used by the > iOS+Mac ViewGestureController and ViewSnapshotStore implementations), > but as far as I can understand it does not need changing because the > currently loaded page is kept. > > The rest of things which need updating are handled properly because > the WebBackForwardList::clear() method itself does calls to the > didChangeBackForwardList() method to make the Page (and possible > other objects) notice the changes. More on this… There is also a “provisionalURL” in the session state, but as a matter of fact, webkit_web_view_restore_session_state() uses WebPageProxy::restoreFromState(…, false), and passing “false” there prevents reading the “.renderTreeSize” and “.provisionalURL“ from the state which is being restored. This me even more confident that just clearing the back-forward list is the only thing needed here :-)

Created attachment 358897 [details] Patch v3 This version takes into account the review comments.

(In reply to Adrian Perez from comment #0) > Unfortunately, the approach above does NOT work: the code that > restores the session state checks whether the current index in > the back/forward is zero, and skips restoring the list in that > case (which happens to be the case for an “empty” state). Is this cross-platform code? It sounds like a bug. If the back/forward list in the session state is empty, the web view's back/forward list should be reset, not ignored.

Comment on attachment 358897 [details] Patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=358897&action=review > Source/WebKit/UIProcess/WebPageProxy.h:811 > RefPtr<API::Navigation> restoreFromSessionState(SessionState, bool navigate); > + void clearSessionState() { backForwardClear(); } I think it should be implemented by passing an empty SessionState into restoreFromSessionState. And if that doesn't work, it's probably a bug in restoreFromSessionState. We'll have to see what WK2 owners think.

(In reply to Michael Catanzaro from comment #11) > Comment on attachment 358897 [details] > Patch v3 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=358897&action=review > > > Source/WebKit/UIProcess/WebPageProxy.h:811 > > RefPtr<API::Navigation> restoreFromSessionState(SessionState, bool navigate); > > + void clearSessionState() { backForwardClear(); } > > I think it should be implemented by passing an empty SessionState into > restoreFromSessionState. And if that doesn't work, it's probably a bug in > restoreFromSessionState. We'll have to see what WK2 owners think. I have been mulling this over the weekend, and I think there is no bug in ::restoreFromSessionState(), because I am under the impression that the idea is to use it right after creating a web view, when there is no previous (nor current) state to maintain. This means that restoring an empty state does not need to do anything, because well, a fresh web view has already an empty state — nothing needs to be done. As for implementing ::clearSessionState() in terms of restoring an empty state using ::restoreFromSessionState() behind the scenes, I think that would be wrong: that would imply passing a state with one element (for the currently loaded page), which would result in “completely replace the state with the passed one, do a page reload of the active item” which is very different from “drop all items from the state which are not the current page” (which is the behaviour implemented by the proposed patch).

(In reply to Adrian Perez from comment #12) > I have been mulling this over the weekend, and I think there is no bug > in ::restoreFromSessionState(), because I am under the impression that > the idea is to use it right after creating a web view, when there is no > previous (nor current) state to maintain. This means that restoring an > empty state does not need to do anything, because well, a fresh web view > has already an empty state — nothing needs to be done. Hm, that's certainly how Epiphany uses it, but there is no indication in the API documentation that this is the intended use, and I don't see any reason it shouldn't work.... > As for implementing ::clearSessionState() in terms of restoring an empty > state using ::restoreFromSessionState() behind the scenes, I think that > would be wrong: that would imply passing a state with one element (for > the currently loaded page) What, why? You should use a completely empty state. >, which would result in “completely replace > the state with the passed one, do a page reload of the active item” I thought restore state always implies reload

(In reply to Michael Catanzaro from comment #13) > (In reply to Adrian Perez from comment #12) > > I have been mulling this over the weekend, and I think there is no bug > > in ::restoreFromSessionState(), because I am under the impression that > > the idea is to use it right after creating a web view, when there is no > > previous (nor current) state to maintain. This means that restoring an > > empty state does not need to do anything, because well, a fresh web view > > has already an empty state — nothing needs to be done. > > Hm, that's certainly how Epiphany uses it, but there is no indication in the > API documentation that this is the intended use, and I don't see any reason > it shouldn't work.... > > > As for implementing ::clearSessionState() in terms of restoring an empty > > state using ::restoreFromSessionState() behind the scenes, I think that > > would be wrong: that would imply passing a state with one element (for > > the currently loaded page) > > What, why? You should use a completely empty state. No, we want to keep the currently displayed content undisturbed. Which means keeping one item for the current page. > >, which would result in “completely replace > > the state with the passed one, do a page reload of the active item” > > I thought restore state always implies reload It does. So if we pass a state with one item, the back-forward list is replaced, and the current page reloaded... which will cause the web view to show a flash of blank content and jump around when reloading. We don't want a reload. Clearing the back-forward list does exactly what we want: drops all the elements which are not the current one, leaving it with a single element (the current page) without modifying that element nor reloading.

(In reply to Adrian Perez from comment #14) > No, we want to keep the currently displayed content undisturbed. > Which means keeping one item for the current page. OK, one last motivating question: what are you really trying to do? Clear the back/forward list? A function that clears the session state should probably also clear the current page, not leave it undisturbed, because the current page is part of the session state. Right? I'm skeptical because session state includes much more than just back/forward list and even the current page. E.g. scroll position within the page is part of the session state. So a clear function that only clears the back/forward list and has a goal of not reloading the page or resetting the scroll position seems really suspect. > Clearing the back-forward list does exactly what we want: drops all > the elements which are not the current one, leaving it with a single > element (the current page) without modifying that element nor reloading. It sounds like you really want a function to clear just the back/forward list. We have previous bugs about that on Bugzilla where Apple explained we don't want to make it mutable, but perhaps they might have been open to allowing a simple clear operation without allowing other mutations.

I also find it confusing and the session state might include more stuff in the future. If we actually want an API to simply clear the bf list let's add webkit_back_forward_list_clear() or reset(), it might be weird a clear() method that leaves one item.

(In reply to Carlos Garcia Campos from comment #16) > I also find it confusing and the session state might include more stuff in > the future. If we actually want an API to simply clear the bf list let's add > webkit_back_forward_list_clear() or reset(), it might be weird a clear() > method that leaves one item. I like this idea, and in particular calling the method _reset() to avoid giving the idea that the back-forward list will be empty after the call. Thanks for the suggestion!

Comment on attachment 358897 [details] Patch v3 backForwardClear is only accessible from WKBundleClearHistoryForTesting, and I don't think we should expand the use of this functionality. In the past we have consistently found that applications which claim to need to mutate the back/forward list do not actually need to.