Bug 25613 – [GTK] Allow to embed any GtkWidget for embed/object

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Holger Freyther]

Allow to embed any GtkWidget (with and without GdkWindow) into HTML by using the embed/object tags. This patch will allow to react to an ordinary plugin load by emitting a signal and embedding the returned GtkWidget.

[Holger Freyther]

Created attachment 30096 [details]
Implement Widget::setIsSelected by setting a property (when it exists)

Provide a implementation for Widget::setIsSelected using the GObject properties.

[Holger Freyther]

Created attachment 30097 [details]
Change FrameLoaderClient::createPlugin to emit a signal and maybe take a GtkWidget

Implement this feature. Add a new class to swallow any GtkWidget and embed it into the ScrollView of WebCore.

[Holger Freyther]

Created attachment 30098 [details]
Usage example

A simple example how to embed the gtk scribble example as flash replacement.

[Xan Lopez]

Comment on attachment 30097 [details]
Change FrameLoaderClient::createPlugin to emit a signal and maybe take a GtkWidget

> +template <> void freeOwnedGPtr<GHashTable>(GHashTable* ptr)
> +{
> +    if (ptr)
> +        g_hash_table_unref(ptr);
> +}
> +
>  } // namespace WTF
> diff --git a/JavaScriptCore/wtf/GOwnPtr.h b/JavaScriptCore/wtf/GOwnPtr.h
> index bbb793a..8d03ff2 100644
> --- a/JavaScriptCore/wtf/GOwnPtr.h
> +++ b/JavaScriptCore/wtf/GOwnPtr.h
> @@ -35,6 +35,7 @@ namespace WTF {
>      template<> void freeOwnedGPtr<GMutex>(GMutex*);
>      template<> void freeOwnedGPtr<GPatternSpec>(GPatternSpec*);
>      template<> void freeOwnedGPtr<GDir>(GDir*);
> +    template<> void freeOwnedGPtr<GHashTable>(GHashTable*);
>  
>      template <typename T> class GOwnPtr : Noncopyable {
>      public:

I'm going to be a pain in the ass and say that this should go in a separate commit :D

The rest of the patch looks good to me, but since it's new API I'll wait for Gustavo's review to give r+ (we have a rule now to require two reviews for new API).

[Xan Lopez]

Comment on attachment 30096 [details]
Implement Widget::setIsSelected by setting a property (when it exists)

> +    /* See if it has a widget-set-is-selected property and set it */
> +    GParamSpec* spec = g_object_class_find_property(G_OBJECT_GET_CLASS(platformWidget()),
> +                                                    "widget-set-is-selected");

I wonder if we should prefix the property name with 'webkit-' or something like that, to avoid potential collisions.

[Holger Freyther]

> I'm going to be a pain in the ass and say that this should go in a separate
> commit :D

no problem. the addition doesn't stand by itself though, without the rest of the patch no one is using this template specialization, without a user/benefitor the change doesn't make sense... at least this was my logic.


> 
> The rest of the patch looks good to me, but since it's new API I'll wait for
> Gustavo's review to give r+ (we have a rule now to require two reviews for new
> API).

Take your time, I have not put much thinking into it but from using it for a bit... what if you don't want a certain plugin to load but don't want to provide a GtkWidget? create-plugin will not allow you to do this... maybe you want to have another signal, a different API.... or such.. :)

[Holger Freyther]

(In reply to comment #5)
> (From update of attachment 30096 [details] [review])
> > +    /* See if it has a widget-set-is-selected property and set it */
> > +    GParamSpec* spec = g_object_class_find_property(G_OBJECT_GET_CLASS(platformWidget()),
> > +                                                    "widget-set-is-selected");
> 
> I wonder if we should prefix the property name with 'webkit-' or something like
> that, to avoid potential collisions.

webkit-widget-set-is-selected is a bit long? So far http://www.google.com/codesearch?q=widget-set-is-selected&hl=en&btnG=Search+Code is not listing any code, and no widget within gtk itself has such a property. I was thinking about adding a prefix but then thought that

     a) having the concept of a selectable widget
     b) embedding it into WebKit

happen at the same time is highly unlikely... but maybe we should just be safe...

[Christian Dywan]

Would passing a dummy widget work, as a means of disabling a particular plugin? Together with a way to find out about all supported plugins, this might be enough. For example passing a GtkImage containing a "Disabled" icon.

I like the simplicity of this approach.

[Xan Lopez]

(In reply to comment #6)
> Take your time, I have not put much thinking into it but from using it for a
> bit... what if you don't want a certain plugin to load but don't want to
> provide a GtkWidget? create-plugin will not allow you to do this... maybe you
> want to have another signal, a different API.... or such.. :)
> 

IMHO that should be solved with a generic API to block the loading of any resource (which we need anyway for things like adblock).

[Xan Lopez]

(In reply to comment #7)
> webkit-widget-set-is-selected is a bit long? So far
> http://www.google.com/codesearch?q=widget-set-is-selected&hl=en&btnG=Search+Code
> is not listing any code, and no widget within gtk itself has such a property. I
> was thinking about adding a prefix but then thought that
> 
>      a) having the concept of a selectable widget
>      b) embedding it into WebKit
> 
> happen at the same time is highly unlikely... but maybe we should just be
> safe...
> 

I think we should rather be safe than sorry :)

[Xan Lopez]

And another comment, shouldn't the signal be better called "create-plugin-widget"? "create-plugin" seems a bit too generic, this is only related to widgets acting as plugins.

[Holger Freyther]

(In reply to comment #10)
> (In reply to comment #7)
> > webkit-widget-set-is-selected is a bit long? So far
> > http://www.google.com/codesearch?q=widget-set-is-selected&hl=en&btnG=Search+Code
> > is not listing any code, and no widget within gtk itself has such a property. I
> > was thinking about adding a prefix but then thought that
> > 
> >      a) having the concept of a selectable widget
> >      b) embedding it into WebKit
> > 
> > happen at the same time is highly unlikely... but maybe we should just be
> > safe...
> > 
> 
> I think we should rather be safe than sorry :)
> 

hehe. The question is what namespace to use? The namespace of the concept (selectable widgets)? Or the namespace of the library? Certainly using webkit- is the most safe thing we can do, is it the best one?

[Holger Freyther]

(In reply to comment #9)
> (In reply to comment #6)
> > Take your time, I have not put much thinking into it but from using it for a
> > bit... what if you don't want a certain plugin to load but don't want to
> > provide a GtkWidget? create-plugin will not allow you to do this... maybe you
> > want to have another signal, a different API.... or such.. :)
> > 
> 
> IMHO that should be solved with a generic API to block the loading of any
> resource (which we need anyway for things like adblock).


Right. we are scratching the surface of several topics here:
  1.) DOM API (I would love to see something like QWebElement here). To decide if an element should be loaded or removed from the DOM.

  2.) PluginDatabase and PluginFactory interfaces/classes. The app should have the power to decide which plugins are known, which plugins to load...

  3.) Something I just forgot...


From my point of view this simple signal is meant for people wanting to integrate widgets into a WebKitWebView, one side product is an easy to develop flash blocker, the main use case is more like the scribble example though.

[Gustavo Noronha (kov)]

Comment on attachment 30096 [details]
Implement Widget::setIsSelected by setting a property (when it exists)

> +        Implement Widget::setIsSelected for Gtk+ by searching
> +        for a signal of the name widget-set-is-selected and if
> +        this signal is available in the instance we have then
> +        emit it. The signal signature does expect one gboolean
> +        parameter and to return nothing.

^^

property.

> +    g_object_set(platformWidget(), "widget-set-is-selected", isSelected, NULL);

I'd name it 'webkit-widget-is-selected'. I think we want to make the library be the namespace here, because the widget is selected in the context of our library; 'set' is something we would expect to see in a method name, not in a property IMO. With this change, r=me. I'm tagging r+ since Xan already reviewed and suggested adding webkit-, as well. I don't think he'll opppose to s/set-//.

[Gustavo Noronha (kov)]

Comment on attachment 30097 [details]
Change FrameLoaderClient::createPlugin to emit a signal and maybe take a GtkWidget

> +    webkit_web_view_signals[PLUGIN_WIDGET] = g_signal_new("create-plugin",

I'm with Xan here. "create-plugin-widget" sounds good. I think this API is good for the use case it intends to cover, and we will want to investigate how to block specific resources with the help of the DataSource API, I think. r=me on the API with that change (and remember to update the property name in the doc string), I'll poke Xan to update the review flag here =P.

[Xan Lopez]

Comment on attachment 30097 [details]
Change FrameLoaderClient::createPlugin to emit a signal and maybe take a GtkWidget

r=me with the name change discussed (and don't forget to change the Since: version number)

[Holger Freyther]

Landed in r44182 and r44183. I should have updated the property name, the changelog to refer to a property name, the signal to create-plugin-widget, the API documentation, the reference to the property and have added a Since 1.1.8. Something I forgot? Please reopen the bug if that is the case.