Summary: | [GTK] Add API to get plugins to WebKit2 GTK+ | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Carlos Garcia Campos <cgarcia> | ||||
Component: | WebKit2 | Assignee: | Nobody <webkit-unassigned> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | andersca, gustavo, kling, mrobinson, sam, webkit.review.bot, xan.lopez | ||||
Priority: | P2 | Keywords: | Gtk | ||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | PC | ||||||
OS: | Linux | ||||||
Attachments: |
|
Description
Carlos Garcia Campos
2012-05-14 04:34:24 PDT
Created attachment 141692 [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 141692 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=141692&action=review > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:273 > +static void webkitWebContextGetPluginThread(GSimpleAsyncResult* result, GObject* object, GCancellable*) Nit: extra space before GCancellable. > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:299 > + g_simple_async_result_run_in_thread(result.get(), webkitWebContextGetPluginThread, G_PRIORITY_DEFAULT, cancellable); Do you mind explaining why you call pluginInfoStore().plugins() in a thread? Why not simply make this method synchronous and call pluginInfoStore().plugins() right here? I worry because I'm not sure that the WebContext is thread-safe. (In reply to comment #3) > (From update of attachment 141692 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=141692&action=review > > > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:273 > > +static void webkitWebContextGetPluginThread(GSimpleAsyncResult* result, GObject* object, GCancellable*) > > Nit: extra space before GCancellable. oh, ok > > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:299 > > + g_simple_async_result_run_in_thread(result.get(), webkitWebContextGetPluginThread, G_PRIORITY_DEFAULT, cancellable); > > Do you mind explaining why you call pluginInfoStore().plugins() in a thread? Why not simply make this method synchronous and call pluginInfoStore().plugins() right here? Because it's blocking and slow. > I worry because I'm not sure that the WebContext is thread-safe. pluginInfoStore().plugins() is thread-safe. (In reply to comment #4) > pluginInfoStore().plugins() is thread-safe. I don't see any Mutexes in the PluginInfoStore and it looks like other methods call loadPluginsIfNecessary. I think I just need confirmation from the author of PluginInfoStore::plugins() is thread-safe to review this patch. (In reply to comment #5) > (In reply to comment #4) > > > pluginInfoStore().plugins() is thread-safe. > > I don't see any Mutexes in the PluginInfoStore and it looks like other methods call loadPluginsIfNecessary. I think I just need confirmation from the author of PluginInfoStore::plugins() is thread-safe to review this patch. https://trac.webkit.org/browser/trunk/Source/WebKit2/UIProcess/Plugins/PluginInfoStore.cpp#L114 Note also, that the WebContext is calling PluginInfoStore::plugins() from a thread to handle the GetPlugins message (In reply to comment #7) > Note also, that the WebContext is calling PluginInfoStore::plugins() from a thread to handle the GetPlugins message The change (http://trac.webkit.org/changeset/116796) to make this work across threads was made 4 days ago and I'm not following the code right now. Instead of just saying "It's thread safe" perhaps you could have given me a bit more information to help me finish reviewing your patch. (In reply to comment #8) > (In reply to comment #7) > > Note also, that the WebContext is calling PluginInfoStore::plugins() from a thread to handle the GetPlugins message > > The change (http://trac.webkit.org/changeset/116796) to make this work across threads was made 4 days ago and I'm not following the code right now. Instead of just saying "It's thread safe" perhaps you could have given me a bit more information to help me finish reviewing your patch. I posted this patch 2 days ago, when I wrote the patch the code was already thread-safe and I dind't know it had been added just 2 days ago. Committed r117305: <http://trac.webkit.org/changeset/117305> Yeah, this is kool. Though note that I've filed bug 86648 to solidify the thread-safety of it all. :) (In reply to comment #11) > Yeah, this is kool. Though note that I've filed bug 86648 to solidify the thread-safety of it all. :) Okay. Thanks for confirming. Carlos, sorry for my tone above. Bonus points for making this API asynchronous. Ideally we'd like to make all access to the plug-in info store happen on a work queue so we don't need any locking whatsoever. (In reply to comment #12) > (In reply to comment #11) > > Yeah, this is kool. Though note that I've filed bug 86648 to solidify the thread-safety of it all. :) > > Okay. Thanks for confirming. Carlos, sorry for my tone above. No problem! :-) |