[Webkit-unassigned] [Bug 115600] [GTK] Allow to save and restore session

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=115600

Michael Catanzaro <mcatanzaro at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #268007|review?                     |review+
              Flags|                            |

--- Comment #13 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 268007
  --> https://bugs.webkit.org/attachment.cgi?id=268007
Rebased to apply on current trunk

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

This gets API approval from me, but please propose it on the mailing list and wait a few days anyway, to give folks a chance to object, even though probably nobody will.

I almost gave this r- for the messy implementation, since it took me a long time to accept that a GVariant of type (qa(ts(ssssasmaytt(ii)dm(sa(uaysxmxmds))av)u)mu) is really the best way to handle this, rather than using nice C++ iostreams like in your previous patch... but Anders is right, we can't use the IPC encoders because our serialization would break whenever the implementation changes. We hardly want to reimplement all the iostream encoders in this file, which is what it would take to use iostreams safely. It's unfortunate that GVariant is so much harder to use than iostreams, but the mess is contained into one file, and you split it up nicely into "nested" functions. And with GVariant, we know the serialization format is not going to change.

My biggest issue with this patch is that we should have some way to detect changes to the implementation of HTTPBody, FrameState, PageState, BackForwardListState, so that we know to update the serialization here. Currently we can detect deletions, since we'd get a compiler error, but if any new state is added to these classes we will miss it. I think you can solve this using messy static_asserts and sizeof to check the size of the structs. Something like:

#if CPU(X86_64) && COMPILER(GCC_OR_CLANG) && DEVELOPER_MODE
static_assert(sizeof(HTTPBody::Element == sizeof(Type) + sizeof(Vector<char>) + sizeof(String) + sizeof(int64_t) + sizeof(Optional<int64_t>) + sizeof(Optional<double>) + sizeof(String));
#endif

I guess that is pretty messy, though.

> Source/WebKit2/ChangeLog:15
> +        session state form the given WebKitWebViewSessionState.

form -> rom

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3754
> +    getPage(webView)->restoreFromSessionState(webkitWebViewSessionStatateGetSessionState(state), false);

You need to fix this typo all over. webkitWebViewSessionStatateGetSessionState -> webkitWebViewSessionStateGetSessionState

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewSessionState.cpp:59
> +static inline unsigned toExternalURLsPolicy(WebCore::ShouldOpenExternalURLsPolicy policy)

I almost thought you did not need these enums (ExternalURLsPolicy, HTMLBodyElementType) or the conversion functions to go with them. After all, you could just do e.g. static_cast<unsigned>(myWebCoreEnumValue). Deleting them would save about 70 lines of code. Of course, you do need them to guarantee the serialization format, in case the WebCore enums get reordered or something. I recommend adding a comment, else someone like me might break this as a "code cleanup."

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestBackForwardList.cpp:285
> +    webkit_web_view_session_state_unref(state);

OK, but this reminds me, you forgot to declare an autocleanup in WebKitAutocleanups.h.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20151230/1f86ec6d/attachment.html>