Bug 117150

Summary: [GTK] Migrate WebKitCookieManager to GTask
Product: WebKit Reporter: Carlos Garcia Campos <cgarcia>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, danw, gustavo, mrobinson, xan.lopez
Priority: P2 Keywords: Gtk
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 117149    
Attachments:
Description Flags
Patch
none
Updated patch according to review comments gustavo: review+

Description Carlos Garcia Campos 2013-06-03 08:41:17 PDT
Migrate WebKitCookieManager to GTask
Comment 1 Carlos Garcia Campos 2013-06-03 08:46:46 PDT
Created attachment 203594 [details]
Patch
Comment 2 WebKit Commit Bot 2013-06-03 08:48:21 PDT
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
Comment 3 Dan Winship 2013-06-03 10:27:50 PDT
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);
Comment 4 Carlos Garcia Campos 2013-06-03 11:16:41 PDT
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!
Comment 5 Carlos Garcia Campos 2013-06-04 00:39:34 PDT
Created attachment 203660 [details]
Updated patch according to review comments
Comment 6 Dan Winship 2013-06-04 06:11:27 PDT
Comment on attachment 203660 [details]
Updated patch according to review comments

everything looks good to me
Comment 7 Carlos Garcia Campos 2013-06-19 00:14:06 PDT
Committed r151723: <http://trac.webkit.org/changeset/151723>