Bug 117153

Summary: [GTK] Migrate WebKitWebContext 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 to get rid of the async data struct
none
Updated patch gustavo: review+

Description Carlos Garcia Campos 2013-06-03 08:55:09 PDT
Migrate WebKitWebContext to GTask
Comment 1 Carlos Garcia Campos 2013-06-03 08:57:22 PDT
Created attachment 203596 [details]
Patch
Comment 2 WebKit Commit Bot 2013-06-03 08:59:35 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:31:38 PDT
Comment on attachment 203596 [details]
Patch

any reason you kept GetPluginsAsyncData rather than just returning the Vector directly?

If there's some problem with returning C++ types from GTask, I'd like to fix it if possible
Comment 4 Carlos Garcia Campos 2013-06-03 11:19:59 PDT
(In reply to comment #3)
> (From update of attachment 203596 [details])
> any reason you kept GetPluginsAsyncData rather than just returning the Vector directly?
> 
> If there's some problem with returning C++ types from GTask, I'd like to fix it if possible

I don't think so, I kept it just because we need a free function anyway, but I guess we can simply allocate the Vector on the heap and use a helper free function as GDestroyNotify to call delete.
Comment 5 Carlos Garcia Campos 2013-06-04 01:30:15 PDT
Created attachment 203663 [details]
Updated patch to get rid of the async data struct
Comment 6 Dan Winship 2013-06-04 06:25:49 PDT
Comment on attachment 203663 [details]
Updated patch to get rid of the async data struct

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

OK. Belatedly I notice that this has the same issue as the other bug; you're passing back one data structure via the GTask, and then converting it into a different data structure in the _finish function, when you could instead just be creating the final data structure in webkitWebContextGetPluginThread() and returning it directly. You'd still need to write your own GDestroyNotify though, so this wouldn't actually simplify anything, it just moves code around. (But stylistically, my theory was that GTask _finish functions should almost always consist of only a call to g_task_propagate_something().

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:498
> +static void deletePluginsVector(void* ptr)
> +{
> +    delete static_cast<Vector<PluginModuleInfo>*>(ptr);
> +}

Hm... I bet you could do cool code-autogenerating things with GTask and templates, a la GRefPtr... :)
Comment 7 Carlos Garcia Campos 2013-06-04 06:36:32 PDT
(In reply to comment #6)
> (From update of attachment 203663 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=203663&action=review
> 
> OK. Belatedly I notice that this has the same issue as the other bug; you're passing back one data structure via the GTask, and then converting it into a different data structure in the _finish function, when you could instead just be creating the final data structure in webkitWebContextGetPluginThread() and returning it directly. You'd still need to write your own GDestroyNotify though, so this wouldn't actually simplify anything, it just moves code around. (But stylistically, my theory was that GTask _finish functions should almost always consist of only a call to g_task_propagate_something().

Thought about it indeed, but tried to reduce the amount of things done in the thread. Also as you say it wouldn't simply the code either.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:498
> > +static void deletePluginsVector(void* ptr)
> > +{
> > +    delete static_cast<Vector<PluginModuleInfo>*>(ptr);
> > +}
> 
> Hm... I bet you could do cool code-autogenerating things with GTask and templates, a la GRefPtr... :)

I tried it adding template <typename T*> void deletePtrFunction(T* ptr);. But it doesn't simply the code either, but rather the opposite with more C++ casts everywhere.
Comment 8 Carlos Garcia Campos 2013-06-04 06:43:09 PDT
(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 203663 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=203663&action=review
> > 
> > OK. Belatedly I notice that this has the same issue as the other bug; you're passing back one data structure via the GTask, and then converting it into a different data structure in the _finish function, when you could instead just be creating the final data structure in webkitWebContextGetPluginThread() and returning it directly. You'd still need to write your own GDestroyNotify though, so this wouldn't actually simplify anything, it just moves code around. (But stylistically, my theory was that GTask _finish functions should almost always consist of only a call to g_task_propagate_something().
> 
> Thought about it indeed, but tried to reduce the amount of things done in the thread. Also as you say it wouldn't simply the code either.

Actually, creating the GList in the thread we avoid an unnecessary Vector copy and allocation, and we get rid of the delete. Also calling webkitPluginCreate() from the thread shouldn't be a problem either. I'll update the patch.
Comment 9 Carlos Garcia Campos 2013-06-04 06:53:37 PDT
(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > (From update of attachment 203663 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=203663&action=review
> > > 
> > > OK. Belatedly I notice that this has the same issue as the other bug; you're passing back one data structure via the GTask, and then converting it into a different data structure in the _finish function, when you could instead just be creating the final data structure in webkitWebContextGetPluginThread() and returning it directly. You'd still need to write your own GDestroyNotify though, so this wouldn't actually simplify anything, it just moves code around. (But stylistically, my theory was that GTask _finish functions should almost always consist of only a call to g_task_propagate_something().
> > 
> > Thought about it indeed, but tried to reduce the amount of things done in the thread. Also as you say it wouldn't simply the code either.
> 
> Actually, creating the GList in the thread we avoid an unnecessary Vector copy and allocation, and we get rid of the delete. Also calling webkitPluginCreate() from the thread shouldn't be a problem either. I'll update the patch.

Vector has to be copied, but still I'll update the patch to create the GList in the thread and simplify _finish().
Comment 10 Carlos Garcia Campos 2013-06-04 08:09:18 PDT
Created attachment 203700 [details]
Updated patch
Comment 11 Carlos Garcia Campos 2013-07-03 01:44:40 PDT
Committed r152344: <http://trac.webkit.org/changeset/152344>