<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><span class="vcard"><a class="email" href="mailto:mcatanzaro@igalia.com" title="Michael Catanzaro <mcatanzaro@igalia.com>"> <span class="fn">Michael Catanzaro</span></a>
</span> changed
<a class="bz_bug_link
bz_status_UNCONFIRMED "
title="UNCONFIRMED - [GTK] Allow to save and restore session"
href="https://bugs.webkit.org/show_bug.cgi?id=115600">bug 115600</a>
<br>
<table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>What</th>
<th>Removed</th>
<th>Added</th>
</tr>
<tr>
<td style="text-align:right;">Attachment #268007 Flags</td>
<td>review?
</td>
<td>review+
</td>
</tr></table>
<p>
<div>
<b><a class="bz_bug_link
bz_status_UNCONFIRMED "
title="UNCONFIRMED - [GTK] Allow to save and restore session"
href="https://bugs.webkit.org/show_bug.cgi?id=115600#c13">Comment # 13</a>
on <a class="bz_bug_link
bz_status_UNCONFIRMED "
title="UNCONFIRMED - [GTK] Allow to save and restore session"
href="https://bugs.webkit.org/show_bug.cgi?id=115600">bug 115600</a>
from <span class="vcard"><a class="email" href="mailto:mcatanzaro@igalia.com" title="Michael Catanzaro <mcatanzaro@igalia.com>"> <span class="fn">Michael Catanzaro</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=268007&action=diff" name="attach_268007" title="Rebased to apply on current trunk">attachment 268007</a> <a href="attachment.cgi?id=268007&action=edit" title="Rebased to apply on current trunk">[details]</a></span>
Rebased to apply on current trunk
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=268007&action=review">https://bugs.webkit.org/attachment.cgi?id=268007&action=review</a>
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.
<span class="quote">> Source/WebKit2/ChangeLog:15
> + session state form the given WebKitWebViewSessionState.</span >
form -> rom
<span class="quote">> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3754
> + getPage(webView)->restoreFromSessionState(webkitWebViewSessionStatateGetSessionState(state), false);</span >
You need to fix this typo all over. webkitWebViewSessionStatateGetSessionState -> webkitWebViewSessionStateGetSessionState
<span class="quote">> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewSessionState.cpp:59
> +static inline unsigned toExternalURLsPolicy(WebCore::ShouldOpenExternalURLsPolicy policy)</span >
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."
<span class="quote">> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestBackForwardList.cpp:285
> + webkit_web_view_session_state_unref(state);</span >
OK, but this reminds me, you forgot to declare an autocleanup in WebKitAutocleanups.h.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>