[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

Carlos Garcia Campos <cgarcia at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #261304|review?                     |review-
              Flags|                            |

--- Comment #4 from Carlos Garcia Campos <cgarcia at igalia.com> ---
Comment on attachment 261304
  --> https://bugs.webkit.org/attachment.cgi?id=261304
Patch

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

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 http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API. 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 bug #84960 for that part. Once that is working, then we can expose it in our API.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3717
> + * webkit_web_view_serialize_session

Missing trailing : here

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3724
> + * Returns: (transfer none): A GBytes buffer containing the state, or NULL.

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.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3726
> + * Since: 2.10

Too late for 2.10, this should be 2.12

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3728
> +GBytes* webkit_web_view_serialize_session(WebKitWebView *webView)

WebKitWebView *webView -> WebKitWebView* webView

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3730
> +    g_return_val_if_fail(WEBKIT_IS_WEB_VIEW(webView), NULL);

NULL -> nullptr

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3733
> +    g_return_val_if_fail(!!page, NULL);

Don't ue g_return macros for this. Use an ASSERT or and early return.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3735
> +    SessionState x = page->sessionState(0);

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: http://www.webkit.org/coding/coding-style.html

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3740
> +    GBytes* buf = g_bytes_new_take(encoder->buffer(), encoder->bufferSize());
> +    return buf;

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.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3744
> + * webkit_web_view_restore_session

Missing trailing : here

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3747
> + * @navigate: a #gboolean that if #TRUE will trigger navigation to the site set in the state

#TRUE -> %TRUE

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3751
> + * Since: 2.10

Too late for 2.10, this should be 2.12

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3753
> +void webkit_web_view_restore_session(WebKitWebView *webView, GBytes *state, gboolean navigate)

WebKitWebView *webView -> WebKitWebView* webView
GBytes *state -> GBytes* state

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3758
> +    WebPageProxy* page = getPage(webView);
> +    g_return_if_fail(!!page);

Don't ue g_return macros for this. Use an ASSERT or and early return.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3762
> +        (uint8_t *) g_bytes_get_data(state, &size), size);

Use C++ style casts.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3764
> +    SessionState session_state;

session_state -> sessionState

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3766
> +    page->restoreFromSessionState(session_state, navigate);

WTF::move(session_state)

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3767
> +}

The decoder is also leaked here.

-- 
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/20150917/e075ffc0/attachment.html>