Bug 117150 – [GTK] Migrate WebKitCookieManager to GTask

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Carlos Garcia Campos]

Migrate WebKitCookieManager to GTask

[Carlos Garcia Campos]

Created attachment 203594 [details]
Patch

[WebKit Commit Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Dan Winship]

Comment on attachment 203594 [details]
Patch

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

[Carlos Garcia Campos]

Comment on attachment 203594 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/gtk/WebKitCookieManager.cpp:193
>> +    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...)

Good catch, the user should indeed check the error, we were returning WEBKIT_COOKIE_POLICY_ACCEPT_NO_THIRD_PARTY before to use a valid WebKitCookieAcceptPolicy, because it's the default one. I'll update it.

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

Not exactly, GRefPtr calls g_ptr_arry_unref not _free, but what I didn't know is that free also decreases the reference counter. Your proposal is indeed less confusing and cleaner, I'll do that, thanks!

[Carlos Garcia Campos]

Created attachment 203660 [details]
Updated patch according to review comments

[Dan Winship]

Comment on attachment 203660 [details]
Updated patch according to review comments

everything looks good to me

[Carlos Garcia Campos]

Committed r151723: <http://trac.webkit.org/changeset/151723>