ssia
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! :-)