<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><span class="vcard"><a class="email" href="mailto:cgarcia@igalia.com" title="Carlos Garcia Campos <cgarcia@igalia.com>"> <span class="fn">Carlos Garcia Campos</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 #261304 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#c4">Comment # 4</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:cgarcia@igalia.com" title="Carlos Garcia Campos <cgarcia@igalia.com>"> <span class="fn">Carlos Garcia Campos</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=261304&action=diff" name="attach_261304" title="Patch">attachment 261304</a> <a href="attachment.cgi?id=261304&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=261304&action=review">https://bugs.webkit.org/attachment.cgi?id=261304&action=review</a>
Thanks for the patch. This adds new API, so it probably requires some discussion in the mailing list first. The patch should include unit tests to cover the new API added. See <a href="http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API">http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API</a>. If we are going to expose the session state API, I think we should first implement the internal implementation and enabling the cross-platform unit tests to make sure it works. I think that adding a LegacySessionStateCoding implementation could be enough, and then enable the Tests in Tools/TestWebKitAPI/Tests/WebKit2 that use the WKSession API. We could use <a class="bz_bug_link
bz_status_NEW "
title="NEW - [GTK] Test TestWebKitAPI/WebKit2/TestRestoreSessionStateContainingFormData fails"
href="show_bug.cgi?id=84960">bug #84960</a> for that part. Once that is working, then we can expose it in our API.
<span class="quote">> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3717
> + * webkit_web_view_serialize_session</span >
Missing trailing : here
<span class="quote">> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3724
> + * Returns: (transfer none): A GBytes buffer containing the state, or NULL.</span >
This says transfer none, but returns a newly created GBytes that we don't free.
GBytes -> #GBytes
NULL -> %NULL
We should also document why this can return NULL.
<span class="quote">> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3726
> + * Since: 2.10</span >
Too late for 2.10, this should be 2.12
<span class="quote">> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3728
> +GBytes* webkit_web_view_serialize_session(WebKitWebView *webView)</span >
WebKitWebView *webView -> WebKitWebView* webView
<span class="quote">> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3730
> + g_return_val_if_fail(WEBKIT_IS_WEB_VIEW(webView), NULL);</span >
NULL -> nullptr
<span class="quote">> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3733
> + g_return_val_if_fail(!!page, NULL);</span >
Don't ue g_return macros for this. Use an ASSERT or and early return.
<span class="quote">> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3735
> + SessionState x = page->sessionState(0);</span >
Do not use x for a variable name, use a word that better defines what this is for. And use nullptr instead o 0. See the WebKit coding style: <a href="http://www.webkit.org/coding/coding-style.html">http://www.webkit.org/coding/coding-style.html</a>
<span class="quote">> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3740
> + GBytes* buf = g_bytes_new_take(encoder->buffer(), encoder->bufferSize());
> + return buf;</span >
We don't need the local variable. The ArgumentEncoder previously allocated is leaked here. And the data can't be taken by the GBytes, because it's owned by the ArgumentEncoder and because it will be freed by the GBytes using g_free, but was allocated by WebKit using bmalloc. In any case, I don't think we should use the ArgumentEncoder here this way. We should either return an object wrapping the SessionState, or if we really want to return encoded data, use the encodeLegacySessionState() method.
<span class="quote">> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3744
> + * webkit_web_view_restore_session</span >
Missing trailing : here
<span class="quote">> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3747
> + * @navigate: a #gboolean that if #TRUE will trigger navigation to the site set in the state</span >
#TRUE -> %TRUE
<span class="quote">> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3751
> + * Since: 2.10</span >
Too late for 2.10, this should be 2.12
<span class="quote">> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3753
> +void webkit_web_view_restore_session(WebKitWebView *webView, GBytes *state, gboolean navigate)</span >
WebKitWebView *webView -> WebKitWebView* webView
GBytes *state -> GBytes* state
<span class="quote">> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3758
> + WebPageProxy* page = getPage(webView);
> + g_return_if_fail(!!page);</span >
Don't ue g_return macros for this. Use an ASSERT or and early return.
<span class="quote">> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3762
> + (uint8_t *) g_bytes_get_data(state, &size), size);</span >
Use C++ style casts.
<span class="quote">> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3764
> + SessionState session_state;</span >
session_state -> sessionState
<span class="quote">> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3766
> + page->restoreFromSessionState(session_state, navigate);</span >
WTF::move(session_state)
<span class="quote">> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3767
> +}</span >
The decoder is also leaked here.</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>