Bug 159631 – [GTK] Add webkit_context_menu_item_new_from_gaction

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

[Michael Catanzaro]

It should be possible to create a WebKitContextMenuItem by passing a GAction rather than a GtkAction, so that applications can create WebKitContextMenuItems without a spew of deprecation warnings. Our GSoC student Iulian has this API already working on an Epiphany sidebranch.

[Iulian Radu]

Created attachment 288040 [details]
[GTK] Add webkit_context_menu_item_new_from_gaction

This allows creating a WebKitContextMenuItem from a GAction. Besides the GAction, the function also receives an optional GVariant parameter that will be passed to the g_action_activate () function when the item is activated and a label to be displayed for the item.

[Iulian Radu]

Created attachment 288043 [details]
Patch

[Michael Catanzaro]

Comment on attachment 288043 [details]
Patch

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

Great job!

r+ means review pass, since my comments are all minor. cq- means commit-queue denied. You'll need to re-upload this patch to address my comments, but you can manually write "Reviewed by Michael Catanzaro" and don't need to set the r? flag again, just cq?. Still, since this patch adds new API, it needs to be approved by a second GTK+ reviewer before we can commit it. Carlos Garcia will look at it soon.

Also, I forgot to suggest this earlier, but we should also deprecate webkit_context_menu_item_new(), since it's impossible to use without using deprecated GtkAction. You can look at webkit_web_context_set_disk_cache_directory() for an example of how to deprecate a method; it would look like this:

Deprecated: 2.16. Use webkit_context_menu_item_new_from_gaction() instead.

> Source/WebKit2/UIProcess/API/gtk/WebKitContextMenuItem.cpp:199
> +static void actionDataFree(ActionData* actionData, GObject* object)

I like the documented name of the second parameter: GObject* whereTheObjectWas. It makes it more clear that the object is being finalized and is not safe to use. But actually, you don't need this parameter at all (see my next comment), so you should leave it unnamed to avoid unused variable compiler warnings:

static void actionDataFree(ActionData* actionData, GObject*)

> Source/WebKit2/UIProcess/API/gtk/WebKitContextMenuItem.cpp:202
> +    g_return_if_fail(G_IS_OBJECT(object));

Hm, I'm slightly surprised this check passes, since object is being finalized after all. I would remove it. You don't use object anywhere in this function, and you know it's being finalized, so why check to see if it's valid?

> Source/WebKit2/UIProcess/API/gtk/WebKitContextMenuItem.cpp:205
> +    if (actionData->parameter)
> +        g_variant_unref(actionData->parameter);

This works, but try something more idiomatic: use a GRefPtr<GVariant> in the ActionData struct. Then you can delete these lines. Since you're not allocating the ActionData struct with normal C++ new and the struct would no longer contain only plain-old-data (POD) types, you'll have to manually invoke ActionData's destructor right before freeing the memory:

actionData->~ActionData();
g_slice_free(ActionData, actionData); // or free it some other way as appropriate, see comments below

> Source/WebKit2/UIProcess/API/gtk/WebKitContextMenuItem.cpp:234
> + * Creates a new #WebKitContextMenuItem for the given action using the given @label.

I would write: Creates a new #WebKitContextMenuItem for @action using the label @label.

> Source/WebKit2/UIProcess/API/gtk/WebKitContextMenuItem.cpp:240
> +WebKitContextMenuItem* webkit_context_menu_item_new_from_gaction(GAction* action, GVariant* parameter, const gchar* label)

OK, time for my only serious concern with this patch: what are the ownership semantics of the GAction parameter? Since you do not have any transfer annotation, the default is transfer full. Is that what you intended? Is it the best choice here? Since it is transfer full, you have to free it in actionDataFree, but it looks like you don't do that. If you intended it to be transfer none, you need to add the annotation.

> Source/WebKit2/UIProcess/API/gtk/WebKitContextMenuItem.cpp:245
> +    ActionData* actionData = g_slice_new(ActionData);

Instead of using GLib's "fast" allocator, which isn't very good, it's better to use WebKit's faster one (bmalloc):

ActionData* actionData = static_cast<ActionData*>(fastMalloc(sizeof(ActionData)));

Then you would free it with fastFree(actionData).

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestContextMenu.cpp:542
> +class ContextMenuCustomGActionTest: public ContextMenuTest {

Leave a space before the colon. I see you matched the style of the rest of this file, which is normally the right thing to do, but in this case the other examples in this file were wrong. :)

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestContextMenu.cpp:549
> +        : m_itemToActivateLabel(0)
> +        , m_activated(false)
> +        , m_toggled(false)

In new code we initialize these inline-style instead of in the constructor. I'll show you down below.

(And be careful to use nullptr for pointers, not 0.)

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestContextMenu.cpp:571
> +        return 0;

Do you really need the return for it to compile? GCC isn't smart enough to see it's unreachable? If you have to keep it, return nullptr rather than 0, since the return value is a pointer type.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestContextMenu.cpp:587
> +        return FALSE;

return G_SOURCE_REMOVE (readable alias for FALSE)

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestContextMenu.cpp:592
> +        m_activated = m_toggled = false;

I think I prefer to write this on two lines.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestContextMenu.cpp:615
> +    void setAction(GAction* action, GVariant *parameter, const char* label)
> +    {
> +        m_action = action;

Add another check: assertObjectIsDeletedWhenTestFinishes(G_OBJECT(action)). Does it pass?

Also, careful to write GVariant* parameter rather than GVariant *parameter while we're in WebKit code rather than GNOME code.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestContextMenu.cpp:629
> +    const char* m_label;
> +    const char* m_itemToActivateLabel;
> +    bool m_activated;
> +    bool m_toggled;

const char* m_label { nullptr };
const char* m_itemToActivateLabel { nullptr };
bool m_activated { false };
bool m_toggled { false };

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestContextMenu.cpp:1146
> +    ContextMenuCustomGActionTest::add("WebKitWebView", "gaction-populate-menu", testContextMenuGActionPopulateMenu);

Hm, how about "populate-menu-with-gaction" and "testContextMenuPopulateMenuWithGAction"?

[Michael Catanzaro]

Comment on attachment 288043 [details]
Patch

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

> Source/WebKit2/UIProcess/API/gtk/WebKitContextMenuItem.cpp:216
> +    g_action_activate(actionData->action, actionData->parameter);
> +    actionData->parameter = nullptr;

If you use a GRefPtr for ActionData::parameter, then you would write here:

g_action_activate(actionData->action, actionData->parameter.leakRef());

And you would not need to set actionData->parameter = nullptr (leakRef will take care of that).

[Carlos Garcia Campos]

(In reply to comment #3)
> Comment on attachment 288043 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=288043&action=review
> 
> Great job!
> 
> r+ means review pass, since my comments are all minor. cq- means
> commit-queue denied. You'll need to re-upload this patch to address my
> comments, but you can manually write "Reviewed by Michael Catanzaro" and
> don't need to set the r? flag again, just cq?. Still, since this patch adds
> new API, it needs to be approved by a second GTK+ reviewer before we can
> commit it. Carlos Garcia will look at it soon.

I'm not sure about this API, TBH. It's just creating a GtkAction from a GAction and then calling webkit_context_menu_item_new() with the GtkAction, something that applications can do. I agree it would be convenient if that code is duplicated in all applications, but I don't think there's any application currently using this.

> Also, I forgot to suggest this earlier, but we should also deprecate
> webkit_context_menu_item_new(), since it's impossible to use without using
> deprecated GtkAction. You can look at
> webkit_web_context_set_disk_cache_directory() for an example of how to
> deprecate a method; it would look like this:
> 
> Deprecated: 2.16. Use webkit_context_menu_item_new_from_gaction() instead.

No, we can't. To do that we need to rework the internal context menu code to work with GAction (I don't even know if that's possible), and then deprecate the public API using GtkAction. With this patch if you create a context menu item from a GAction, the only way to get the action is using webkit_context_menu_item_get_action(), that returns a GtkAction. This is also quite confusing. It wouldn't be confusing in applications code, since they know they are actually creating a GtkAction from the GAction.

[Michael Catanzaro]

(In reply to comment #5)
> I'm not sure about this API, TBH. It's just creating a GtkAction from a
> GAction and then calling webkit_context_menu_item_new() with the GtkAction,
> something that applications can do. I agree it would be convenient if that
> code is duplicated in all applications, but I don't think there's any
> application currently using this.

It's quite a complex thing to expect applications to do, though. This code is being moved from Epiphany. We want to stop using GtkAction so we can drop -Wno-deprecated-declarations from our build. Surely, any application creating its own WebKitContextMenuItems would want this API. It might be that only Epiphany is doing that right now, but this is generally useful.

> No, we can't. To do that we need to rework the internal context menu code to
> work with GAction (I don't even know if that's possible), and then deprecate
> the public API using GtkAction. With this patch if you create a context menu
> item from a GAction, the only way to get the action is using
> webkit_context_menu_item_get_action(), that returns a GtkAction.

OK, you're right about that, I didn't notice this problem. :( So deprecating the GtkAction version only makes sense if we also deprecate webkit_context_menu_item_get_action, even though we don't have a replacement for that. I don't think it matters too much whether we deprecate these or not, but I would, since GtkAction itself is deprecated and we have no choice but to remove these functions from our GTK+ 4 API.

Another issue is that the first unstable GTK+ 4 release is right around the corner, and GtkAction is going away. This patch is a good band-aid for GTK+ 3 API users, but for GTK+ 4 we need to completely rewrite WebKitContextMenu/WebKitContextMenuItem to no longer use GtkAction. We will need two parallel implementations depending on which version of GTK+ we compile with. (The function added in this patch clearly only makes sense for our GTK+ 3 API.)

[Michael Catanzaro]

Mike, do you want this for Geary?

I think I prefer to proceed with this patch, deprecating also webkit_context_menu_item_get_action() as well. Applications are going to have a very rough transition from our GTK+ 3 API to our future GTK+ 4 API if we don't provide GAction support in the GTK+ 3 API.

[Michael Gratton]

(In reply to comment #7)
> Mike, do you want this for Geary?
> 
> I think I prefer to proceed with this patch, deprecating also
> webkit_context_menu_item_get_action() as well. Applications are going to
> have a very rough transition from our GTK+ 3 API to our future GTK+ 4 API if
> we don't provide GAction support in the GTK+ 3 API.

Yup, I was just discussing this with nielsdg on #geary. He's currently cleaning up the composer implementation, with an eye to the WK2 port. We'd like to use GMenu and GActions if at all possible, but even just GAction support would be great.

[Carlos Garcia Campos]

(In reply to comment #7)
> Mike, do you want this for Geary?
> 
> I think I prefer to proceed with this patch, deprecating also
> webkit_context_menu_item_get_action() as well. Applications are going to
> have a very rough transition from our GTK+ 3 API to our future GTK+ 4 API if
> we don't provide GAction support in the GTK+ 3 API.

Let's move internally to GAction first, I don't want to deprecate get_action without a replacement.

[Michael Catanzaro]

Comment on attachment 288043 [details]
Patch

Changing to r- since it's been several days and my question about ownership of the GAction remains unresolved.

[Jan]

Just wanted to drop by and say:
FeedReader (http://jangernert.github.io/FeedReader/) is also using this. So any work porting to GAction is greatly appreciated :)

[Jan]

Sorry for posting again. Just want to mention 120636 looks like a duplicate report:
https://bugs.webkit.org/show_bug.cgi?id=120636

[Carlos Garcia Campos]

We use GAction internally now, Julian, do you plan to update the patch? Now we should also add a get_gaction method and deprecate the old ones.

[Cédric Bellegarde]

This method is not available from GObject introspection.

[Michael Catanzaro]

What do you mean by that? I see the appropriate gtk-doc metadata in the comment above the function. We don't understand introspection very well, so please clearly describe the problem with introspection in this patch.

Anyway, this is going to need to be reworked anyway in accordance with comment #13.

[Cédric Bellegarde]

When using python, we can only use Gtk.Action, this new method is not available.
Should I report to GNOME devs?

[Michael Catanzaro]

Have you applied this patch locally? The patch was rejected, so it is not expected to be available otherwise. :)

[Carlos Garcia Campos]

Created attachment 310648 [details]
Patch

[Michael Catanzaro]

Comment on attachment 310648 [details]
Patch

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

Nice! Reasoning about actions is tricky.

We really ought to deprecate webkit_context_menu_item_new() and webkit_context_menu_item_get_action() in this patch. Deprecations are an optional service to let developers know that the function is going away eventually and ease porting to our future GTK+ API. It doesn't mean developers have to immediately stop using them; of course we'll keep them working in our GTK+ 3 API forever. But we are for sure that the GtkAction version of this function is going to be gone in our GTK+ 4 API, so best let developers know that now. r=me if you add deprecations for these two functions (both header file attributes and introspection annotations).

> Source/WebKit2/UIProcess/gtk/WebContextMenuProxyGtk.cpp:84
> +        if (menuItem.action() < ContextMenuItemBaseApplicationTag) {

Why did you have to add this conditional?

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #19)
> Comment on attachment 310648 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=310648&action=review
> 
> Nice! Reasoning about actions is tricky.

And it was so easy with GtkAction...

> We really ought to deprecate webkit_context_menu_item_new() and
> webkit_context_menu_item_get_action() in this patch. Deprecations are an
> optional service to let developers know that the function is going away
> eventually and ease porting to our future GTK+ API. It doesn't mean
> developers have to immediately stop using them; of course we'll keep them
> working in our GTK+ 3 API forever. But we are for sure that the GtkAction
> version of this function is going to be gone in our GTK+ 4 API, so best let
> developers know that now. r=me if you add deprecations for these two
> functions (both header file attributes and introspection annotations).

Ok, I'll deprecate it.

> > Source/WebKit2/UIProcess/gtk/WebContextMenuProxyGtk.cpp:84
> > +        if (menuItem.action() < ContextMenuItemBaseApplicationTag) {
> 
> Why did you have to add this conditional?

Because glib checks if there are handlers connected to activate signal, and we don't want that glib thinks we are going to handle the activate, because we are not, user is expected to do it (or glib when no handlers are connected). Note that WebPageProxy::contextMenuItemSelected() returns early with the same condition, notifying the client before, but we don't implement customContextMenuItemSelected(), because users can connect to activate of the signal themselves.

[Carlos Garcia Campos]

Committed r217209: <http://trac.webkit.org/changeset/217209>