Since r230812, decodeBackForwardListItemState in WebKitWebViewSessionState.cpp no longer reads the encoded BackForwardList identifier. First, we should figure out if this is bad. Does the BackForwardList still get deserialized properly? It's not clear to me. If it's not bad, we should at least stop encoding the identifier, since it's silly to encode something that's never decoded. To avoid the need to change the serialization format, we could just start encoding 0, for example, with a comment to indicate that it should be dropped if we ever need to bump the serialization format.
(In reply to Michael Catanzaro from comment #0) > Since r230812, decodeBackForwardListItemState in > WebKitWebViewSessionState.cpp no longer reads the encoded BackForwardList > identifier. Right. > First, we should figure out if this is bad. Does the BackForwardList still > get deserialized properly? It's not clear to me. Nah, it's not bad because WebBackForwardList::restoreFromState() assigns a new identifier to the item without checking the saved one. This was done before as well, so I think we were saving the identifier in the state for nothing. Brady, correct me if I'm wrong. > If it's not bad, we should at least stop encoding the identifier, since it's > silly to encode something that's never decoded. To avoid the need to change > the serialization format, we could just start encoding 0, for example, with > a comment to indicate that it should be dropped if we ever need to bump the > serialization format. It should be easy to handle this change, I think.
Created attachment 338868 [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
Comment on attachment 338868 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=338868&action=review > Source/WebKit/UIProcess/API/glib/WebKitWebViewSessionState.cpp:394 > + GRefPtr<GVariant> variant; > + for (unsigned i = 0; sessionStateTypeStringVersions[i]; ++i) { > + sessionStateTypeString = sessionStateTypeStringVersions[i]; > + variant = g_variant_new_from_bytes(G_VARIANT_TYPE(sessionStateTypeString), data, FALSE); > + if (g_variant_is_normal_form(variant.get())) > + break; > + variant = nullptr; > + } You've tested this to ensure it doesn't emit a bunch of warnings or criticals when the type doesn't match?
(In reply to Michael Catanzaro from comment #4) > Comment on attachment 338868 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=338868&action=review > > > Source/WebKit/UIProcess/API/glib/WebKitWebViewSessionState.cpp:394 > > + GRefPtr<GVariant> variant; > > + for (unsigned i = 0; sessionStateTypeStringVersions[i]; ++i) { > > + sessionStateTypeString = sessionStateTypeStringVersions[i]; > > + variant = g_variant_new_from_bytes(G_VARIANT_TYPE(sessionStateTypeString), data, FALSE); > > + if (g_variant_is_normal_form(variant.get())) > > + break; > > + variant = nullptr; > > + } > > You've tested this to ensure it doesn't emit a bunch of warnings or > criticals when the type doesn't match? Yes, of course I tested it. My ephy history was successfully encoded with version 1 (2 is tried first) and decoded with 2.
(In reply to Carlos Garcia Campos from comment #5) > (In reply to Michael Catanzaro from comment #4) > > Comment on attachment 338868 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=338868&action=review > > > > > Source/WebKit/UIProcess/API/glib/WebKitWebViewSessionState.cpp:394 > > > + GRefPtr<GVariant> variant; > > > + for (unsigned i = 0; sessionStateTypeStringVersions[i]; ++i) { > > > + sessionStateTypeString = sessionStateTypeStringVersions[i]; > > > + variant = g_variant_new_from_bytes(G_VARIANT_TYPE(sessionStateTypeString), data, FALSE); > > > + if (g_variant_is_normal_form(variant.get())) > > > + break; > > > + variant = nullptr; > > > + } > > > > You've tested this to ensure it doesn't emit a bunch of warnings or > > criticals when the type doesn't match? > > Yes, of course I tested it. My ephy history was successfully encoded with > version 1 (2 is tried first) and decoded with 2. Ok, the opposite, decoded with 1 and encoded with 2 ...
Committed r231090: <https://trac.webkit.org/changeset/231090>