Summary: | [GTK] Migrate WebKitWebContext to GTask | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Carlos Garcia Campos <cgarcia> | ||||||||
Component: | WebKitGTK | Assignee: | 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
Carlos Garcia Campos
2013-06-03 08:55:09 PDT
Created attachment 203596 [details]
Patch
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 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
(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. Created attachment 203663 [details]
Updated patch to get rid of the async data struct
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... :) (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. (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. (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(). Created attachment 203700 [details]
Updated patch
Committed r152344: <http://trac.webkit.org/changeset/152344> |