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

[bugzilla-daemon at webkit.org]

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





--- Comment #4 from Carlos Garcia Campos <cgarcia at igalia.com>  2013-06-03 11:16:41 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
>> +    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!

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