Bug 117153 – [GTK] Migrate WebKitWebContext to GTask

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

[Carlos Garcia Campos]

Migrate WebKitWebContext to GTask

[Carlos Garcia Campos]

Created attachment 203596 [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 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

[Carlos Garcia Campos]

(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.

[Carlos Garcia Campos]

Created attachment 203663 [details]
Updated patch to get rid of the async data struct

[Dan Winship]

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... :)

[Carlos Garcia Campos]

(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.

[Carlos Garcia Campos]

(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.

[Carlos Garcia Campos]

(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().

[Carlos Garcia Campos]

Created attachment 203700 [details]
Updated patch

[Carlos Garcia Campos]

Committed r152344: <http://trac.webkit.org/changeset/152344>