Bug 117150 - [GTK] Migrate WebKitCookieManager to GTask
Summary: [GTK] Migrate WebKitCookieManager to GTask
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks: 117149
  Show dependency treegraph
 
Reported: 2013-06-03 08:41 PDT by Carlos Garcia Campos
Modified: 2013-06-19 00:14 PDT (History)
5 users (show)

See Also:


Attachments
Patch (8.71 KB, patch)
2013-06-03 08:46 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Updated patch according to review comments (8.57 KB, patch)
2013-06-04 00:39 PDT, Carlos Garcia Campos
gns: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>