Bug 150642 – [GTK] Use CROSS_PLATFORM_CONTEXT_MENUS

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Martin Robinson]

WebKitGTK+ might be the last port to be using the non-cross platform context menus.

03:56:46 PM) andersca: mrobinson: do you know if WebKitGTK uses CROSS_PLATFORM_CONTEXT_MENUS?
(03:56:47 PM) andersca: mrobinson: I think it might not
(03:59:05 PM) mrobinson: andersca: I am not sure, but I'll check now.
(03:59:35 PM) andersca: mrobinson: every other port now uses ContextMenu/ContextMenuItem as pure data objects, and then convert them to platform objects right before showing the menu
(04:00:26 PM) mrobinson: andersca: I don't think GTK+ is using them.
(04:00:57 PM) andersca: mrobinson: should fix that :)

[Anders Carlsson]

What needs to be done is that WebKitContextMenu and WebKitContextMenuItem needs to work directly on WebContextMenuItemData instead of the webcore types.

[Carlos Garcia Campos]

(In reply to comment #0)
> WebKitGTK+ might be the last port to be using the non-cross platform context
> menus.
> 
> 03:56:46 PM) andersca: mrobinson: do you know if WebKitGTK uses
> CROSS_PLATFORM_CONTEXT_MENUS?
> (03:56:47 PM) andersca: mrobinson: I think it might not
> (03:59:05 PM) mrobinson: andersca: I am not sure, but I'll check now.
> (03:59:35 PM) andersca: mrobinson: every other port now uses
> ContextMenu/ContextMenuItem as pure data objects, and then convert them to
> platform objects right before showing the menu
> (04:00:26 PM) mrobinson: andersca: I don't think GTK+ is using them.
> (04:00:57 PM) andersca: mrobinson: should fix that :)

really? grepping I only see win and EFL enabling CROSS_PLATFORM_CONTEXT_MENUS. I'll look at it anyway to switch.

[Carlos Garcia Campos]

Created attachment 264798 [details]
Patch

It was not that easy in the end.

[Michael Catanzaro]

Comment on attachment 264798 [details]
Patch

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

Quite a complex patch... I didn't review the whole thing, just adding a couple of comments.

> Source/WebKit2/Shared/gtk/WebContextMenuItemGtk.cpp:43
> +        return GTK_STOCK_COPY;

Since GtkStock has been deprecated for several years now, you might take this opportunity to get rid of it.

GTK_STOCK_COPY would be replaced with "_Copy", for example.

See: https://docs.google.com/spreadsheets/d/1HavJQRPpMuq-N0GoN1wJR-9KEGXpKy3-NEPpZZkUGJY/pub?output=html

I guess that could be a follow-up patch, though.

> Source/WebKit2/Shared/gtk/WebContextMenuItemGtk.cpp:150
> +void WebContextMenuItemGtk::createGtkActionIfNeeded()

How hard is it to add support for GAction as an alternative to GtkAction?  I wouldn't expect you to do that in this patch, but Arnaud and I are planning to drop use of GtkAction in Epiphany, so we need a GAction API here in order to keep our custom context menu items.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestContextMenu.cpp:-96
> -    bool shouldShowInputMethodsMenu()

Eeep, this was broken for ages and nobody noticed? Anyway, I agree with removing it, but maybe you could do that in a separate patch and backport it to 2.10? It sucks to have broken context menu items.

[Carlos Garcia Campos]

(In reply to comment #4)
> Comment on attachment 264798 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=264798&action=review
> 
> Quite a complex patch... I didn't review the whole thing, just adding a
> couple of comments.
> 
> > Source/WebKit2/Shared/gtk/WebContextMenuItemGtk.cpp:43
> > +        return GTK_STOCK_COPY;
> 
> Since GtkStock has been deprecated for several years now, you might take
> this opportunity to get rid of it.
> 
> GTK_STOCK_COPY would be replaced with "_Copy", for example.
> 
> See:
> https://docs.google.com/spreadsheets/d/1HavJQRPpMuq-N0GoN1wJR-9KEGXpKy3-
> NEPpZZkUGJY/pub?output=html
> 
> I guess that could be a follow-up patch, though.

Our API is based on deprecated API GtkAction, so I don't care if stock actions are deprecated too. I have real bugs and important feature to work on before.

> > Source/WebKit2/Shared/gtk/WebContextMenuItemGtk.cpp:150
> > +void WebContextMenuItemGtk::createGtkActionIfNeeded()
> 
> How hard is it to add support for GAction as an alternative to GtkAction?  I
> wouldn't expect you to do that in this patch, but Arnaud and I are planning
> to drop use of GtkAction in Epiphany, so we need a GAction API here in order
> to keep our custom context menu items.

Last time I checked it was not that easy, it would probably require a glib version bump to ensure it does everything we need, and it affects the public API. So, I don't see the point, GtkAction works very well for us.

> > Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestContextMenu.cpp:-96
> > -    bool shouldShowInputMethodsMenu()
> 
> Eeep, this was broken for ages and nobody noticed? Anyway, I agree with
> removing it, but maybe you could do that in a separate patch and backport it
> to 2.10? It sucks to have broken context menu items.

Yes. Nobody noticed it because nobody uses it. It has been disabled by default in GTK+, and we only show it if gtk-show-input-method-menu setting is enabled. So, in real world we are not showing this menu item enymore, because nobody cares to change that (I don't even know how to change that setting). But it has never actually worked, I think. We build the menu from gtk_im_multicontext_append_menuitems (which is deprecated, of course). From that menu we create ContextMenuItems that extract the GtkAction associated with gtk_activatable_get_related_action, but those menu items no longer have a GtkAction associated (and I don't know they ever had it, to be honest). So, we are showing a menu of items that do nothing, because there's no action to connect to emit the activate signal.

[Carlos Garcia Campos]

(In reply to comment #4)

> > Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestContextMenu.cpp:-96
> > -    bool shouldShowInputMethodsMenu()
> 
> Eeep, this was broken for ages and nobody noticed? Anyway, I agree with
> removing it, but maybe you could do that in a separate patch and backport it
> to 2.10? It sucks to have broken context menu items.

I also removed it because it's a pain to port this code to the new context menu, so I'm not going to port it, and remove it in a follow up patch. Current code doesn't allow context menu items without a GtkAction, which is the case of input methods menu items.

[Michael Catanzaro]

Stock items don't matter much if you like them, it just looked like an easy cleanup to me.

(In reply to comment #5)
> Last time I checked it was not that easy, it would probably require a glib
> version bump to ensure it does everything we need, and it affects the public
> API. So, I don't see the point, GtkAction works very well for us.

For Epiphany, we will need to use GAction and GMenuModel to get can get a popover menu button. We could use GAction for that and GtkAction for the context menus, but that would be messy, and GtkAction is confusing and not used elsewhere in GNOME anymore, so we don't want to.

Basically, I want the ability to create WebKitContextMenuItems based on GActions rather than GtkActions. I am hoping the implementation could be layered on top of the current GtkAction implementation; the goal is to ensure the client doesn't have to use confusing GtkAction, but of course we have to in WebKit. Something I'll look into eventually, so don't worry if you don't want to work on it yourself; I just brought it up since you've clearly spent quite some time with the context menus recently, and you understand GtkAction. Anyway, it's outside the scope of this bug.

[Darin Adler]

Comment on attachment 264798 [details]
Patch

Looks really good to me, but probably should have a reviewer who knows the GTK port.

[Carlos Garcia Campos]

Committed r192247: <http://trac.webkit.org/changeset/192247>