[Webkit-unassigned] [Bug 117150] [GTK] Migrate WebKitCookieManager to GTask

[bugzilla-daemon at webkit.org]

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





--- Comment #3 from Dan Winship <danw at gnome.org>  2013-06-03 10:27:50 PST ---
(From update of attachment 203594)
View in context: https://bugs.webkit.org/attachment.cgi?id=203594&action=review

> Source/WebKit2/UIProcess/API/gtk/WebKitCookieManager.cpp:193
> -    if (g_simple_async_result_propagate_error(simpleResult, error))
> -        return WEBKIT_COOKIE_POLICY_ACCEPT_NO_THIRD_PARTY;
> +    g_return_val_if_fail(g_task_is_valid(result, manager), WEBKIT_COOKIE_POLICY_ACCEPT_NO_THIRD_PARTY);
>  
> -    GetAcceptPolicyAsyncData* data = static_cast<GetAcceptPolicyAsyncData*>(g_simple_async_result_get_op_res_gpointer(simpleResult));
> -    return static_cast<WebKitCookieAcceptPolicy>(data->policy);
> +    return static_cast<WebKitCookieAcceptPolicy>(g_task_propagate_int(G_TASK(result), error));

This means that on error you are now returning -1 rather than WEBKIT_COOKIE_POLICY_ACCEPT_NO_THIRD_PARTY. I suppose the API only worked before if the caller checked the error first anyway, so maybe this doesn't matter. You could get the old behavior back by checking g_task_had_error() explicitly. (I'd been thinking about having either g_task_propagate_enum(), or else g_task_propagate_int_with_default(), to deal with this problem...)

> Source/WebKit2/UIProcess/API/gtk/WebKitCookieManager.cpp:215
> +    g_task_return_pointer(task.get(), returnValue.get(), reinterpret_cast<GDestroyNotify>(g_ptr_array_unref));

Hm... returnValue is a GRefPtr, so doesn't that mean will get g_ptr_array_free()ed when this function exits? That's wrong; you need to hand over ownership of the pointer to the GTask, so that it can free it itself at the appropriate time. But you're doing that as well, so the data is going to get freed twice... It looks like the old version had a .leakRef() in _finish which you lost. It would be better to leak the ref when calling g_task_return_pointer() anyway; the caller doesn't necessarily have to call the _finish function right away, so the array needs to live as long as the task does.

Well, and also, there's no particularly good reason to do the GPtrArray -> char** conversion in _finish rather than here...

So I think how it should work is, here you do:

  g_task_return_pointer(task.get(), g_ptr_array_free(returnValue.leakRef(), FALSE), reinterpret_cast<GDestroyNotify>(g_strfreev));

and then in _finish, just do

  return g_task_propagate_pointer(G_TASK(result), error);

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.