Bug 173417 – [GTK] Stop using GtkAction in WebPopupMenuProxyGtk

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Adrian Perez]

There are two reasons to stop using it:

 - GtkAction has been deprecated in GTK+ 3.10, and most likely it will
   not be available in GTK+ 4.x; GAction is recommended instead.

 - The additional functionality from GtkAction is not used. It should
   be possible to directly construct the needed GtkMenuItems without
   creating a GtkAction first.

[Adrian Perez]

Created attachment 312974 [details]
Patch

[Michael Catanzaro]

Comment on attachment 312974 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=312974&action=review

Nice!

> Source/WebKit2/UIProcess/gtk/WebPopupMenuProxyGtk.cpp:68
>              gtk_menu_shell_append(GTK_MENU_SHELL(m_popup), menuItem);

Is this really transfer full? It's not documented that way at all. I suspect it's a bindings error, but otherwise it looks like the GtkMenuItem has always been leaked here. Can you check it?

[Carlos Garcia Campos]

Comment on attachment 312974 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=312974&action=review

>> Source/WebKit2/UIProcess/gtk/WebPopupMenuProxyGtk.cpp:68
>>              gtk_menu_shell_append(GTK_MENU_SHELL(m_popup), menuItem);
> 
> Is this really transfer full? It's not documented that way at all. I suspect it's a bindings error, but otherwise it looks like the GtkMenuItem has always been leaked here. Can you check it?

Widgets are initially unowned, and adding them to a container always transfers the ownership to the container, the floating ref is consumed by the container.

[Adrian Perez]

(In reply to Carlos Garcia Campos from comment #3)
> Comment on attachment 312974 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=312974&action=review
> 
> >> Source/WebKit2/UIProcess/gtk/WebPopupMenuProxyGtk.cpp:68
> >>              gtk_menu_shell_append(GTK_MENU_SHELL(m_popup), menuItem);
> > 
> > Is this really transfer full? It's not documented that way at all. I suspect it's a bindings error, but otherwise it looks like the GtkMenuItem has always been leaked here. Can you check it?
> 
> Widgets are initially unowned, and adding them to a container always
> transfers the ownership to the container, the floating ref is consumed by
> the container.

Yes, this is what happens here as well.

Just for the sake of completeness, I checked the source code and the
chain of function calls inside GTK+, and indeed g_object_ref_sink ends
up being called as expected:

  gtk_menu_shell_append  [gtkmenushell.c]
    gtk_menu_shell_insert
      gtk_menu_shell_real_insert
        gtk_widget_set_parent  [gtkwidget.c]
          gtk_widget_insert_after
            gtk_widget_reposition_after
              g_object_ref_sink


:-)

[WebKit Commit Bot]

Comment on attachment 312974 [details]
Patch

Clearing flags on attachment: 312974

Committed r218329: <http://trac.webkit.org/changeset/218329>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.