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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Sep 17 02:08:41 PDT 2015


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

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

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


> 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);


> 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>

More information about the webkit-unassigned mailing list