Bug 86356 - [GTK] Add API to get plugins to WebKit2 GTK+
: [GTK] Add API to get plugins to WebKit2 GTK+
Status: RESOLVED FIXED
: WebKit
WebKit2
: 528+ (Nightly build)
: PC Linux
: P2 Normal
Assigned To:
:
: Gtk
:
:
  Show dependency treegraph
 
Reported: 2012-05-14 04:34 PST by
Modified: 2012-05-16 11:43 PST (History)


Attachments
Patch (36.19 KB, patch)
2012-05-14 04:43 PST, Carlos Garcia Campos
mrobinson: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-05-14 04:34:24 PST
ssia
------- Comment #1 From 2012-05-14 04:43:00 PST -------
Created an attachment (id=141692) [details]
Patch
------- Comment #2 From 2012-05-14 04:44:48 PST -------
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 From 2012-05-15 11:51:44 PST -------
(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.

> 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.
------- Comment #4 From 2012-05-15 23:55:29 PST -------
(In reply to comment #3)
> (From update of attachment 141692 [details] [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.
------- Comment #5 From 2012-05-16 09:07:51 PST -------
(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.
------- Comment #6 From 2012-05-16 10:01:01 PST -------
(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
------- Comment #7 From 2012-05-16 10:08:11 PST -------
Note also, that the WebContext is calling PluginInfoStore::plugins() from a thread to handle the GetPlugins message
------- Comment #8 From 2012-05-16 10:16:08 PST -------
(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.
------- Comment #9 From 2012-05-16 10:23:48 PST -------
(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.
------- Comment #10 From 2012-05-16 10:28:51 PST -------
Committed r117305: <http://trac.webkit.org/changeset/117305>
------- Comment #11 From 2012-05-16 10:36:48 PST -------
Yeah, this is kool. Though note that I've filed bug 86648 to solidify the thread-safety of it all. :)
------- Comment #12 From 2012-05-16 10:41:53 PST -------
(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.
------- Comment #13 From 2012-05-16 11:23:34 PST -------
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.
------- Comment #14 From 2012-05-16 11:43:27 PST -------
(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! :-)