Bug 86356 – [GTK] Add API to get plugins to WebKit2 GTK+

• Component: WebKit2
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

ssia

[Carlos Garcia Campos]

Created attachment 141692 [details]
Patch

[WebKit Review 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

[Martin Robinson]

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.

[Carlos Garcia Campos]

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

[Martin Robinson]

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

[Carlos Garcia Campos]

(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

[Carlos Garcia Campos]

Note also, that the WebContext is calling PluginInfoStore::plugins() from a thread to handle the GetPlugins message

[Martin Robinson]

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

[Carlos Garcia Campos]

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

[Carlos Garcia Campos]

Committed r117305: <http://trac.webkit.org/changeset/117305>

[Andreas Kling]

Yeah, this is kool. Though note that I've filed bug 86648 to solidify the thread-safety of it all. :)

[Martin Robinson]

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

[Anders Carlsson]

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.

[Carlos Garcia Campos]

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