Bug 81011 – [GTK] Add ContextMenu API to WebKit2 GTK+ API

• Component: WebKit2
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

As proposed in the mailing list we should add a WebKitWebView::context-menu signal, similar to ::populate-menu in WebKit1, but more flexible to make it easier to customize the proposed menu.

[Carlos Garcia Campos]

Created attachment 131675 [details]
Patch

It adds WebKitContextMenu and WebKitContextMenuItem objects as proposed in the mailing list.

[Carlos Garcia Campos]

Patch doesn't apply because it depends on bug #80600

[Carlos Garcia Campos]

Created attachment 147596 [details]
Patch updated to apply on current git master

[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 147596 [details]
Patch updated to apply on current git master

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

This patch looks pretty good, but I think I'd prefer it WebKitContextMenuItem didn't contain a "live" submenu element (just a cache of the realized version of the ContextMenuItem).

The reason I say this is because I'm not sure that it makes sense to have further modifications of a submenu alter some previously instantiated WebKitContextMenuItem. Instead, it makes sense to me for the code to duplicate the submenu item's contents and not hold on to a reference. This allows code like:

WebKitContentMenuItem* item1= webkit_context_menu_item_new_with_submenu(submenu);

/* make some minor changes to the submenu */
WebKitContentMenuItem* item2 = webkit_context_menu_item_new_with_submenu(submenu);

/* make some minor changes to the submenu */
WebKitContentMenuItem* item3 = webkit_context_menu_item_new_with_submenu(submenu);

> Source/WebKit2/UIProcess/API/gtk/WebKitContextMenu.cpp:106
> +WebKitContextMenu* webkit_context_menu_with_items(GList* items)

Probably best to call this webkit_context_new_menu_with_items to make it clear that it's a constructor. This seems to be the style in the Gnome platform (just searched devhelp for "new_with").

> Source/WebKit2/UIProcess/API/gtk/WebKitContextMenu.cpp:118
> +    g_object_ref_sink(item);
> +    menu->priv->items = g_list_insert(menu->priv->items, item, position);

I think it just makes sense to roll this into webkit_context_menu_insert. I believe the double cast/checks for webkit_context_menu_prepend and webkit_context_menu_append shouldn't be a big deal, but you can always just eliminate them in the outer methods.

> Source/WebKit2/UIProcess/API/gtk/WebKitContextMenu.cpp:126
> + * Adds @item at the beginning of the @menu item list.

Probably just best to day "Adds @item at the beginning of the @menu." as the fact that it's a list is somewhat of an implementation detail. :)

> Source/WebKit2/UIProcess/API/gtk/WebKitContextMenu.cpp:141
> + *
> + * Adds @item at the end of the @menu item list.

Ditto.

> Source/WebKit2/UIProcess/API/gtk/WebKitContextMenu.cpp:157
> + * Inserts @item into the @menu item list at the given position.

Ditto.

> Source/WebKit2/UIProcess/API/gtk/WebKitContextMenu.cpp:177
> + * Moves @item to the given position in the @menu item list.
> + * The first position is 0.

I love how you specified the what happens when you pass weird values to webkit_context_menu_insert. Do you mind doing the same here?

> Source/WebKit2/UIProcess/API/gtk/WebKitContextMenuActions.h:54
> + * @WEBKIT_CONTEXT_MENU_ACTION_SPELLING_GUESS: Guess spelling.

It's probably better to say something like "A proposed replacement for a misspelled word."

> Source/WebKit2/UIProcess/API/gtk/WebKitContextMenuActions.h:55
> + * @WEBKIT_CONTEXT_MENU_ACTION_NO_GUESSES_FOUND: No gesses found.

"An indicator in the menu that spellchecking found no proposed replacements."

> Source/WebKit2/UIProcess/API/gtk/WebKitContextMenuActions.h:56
> + * @WEBKIT_CONTEXT_MENU_ACTION_IGNORE_SPELLING: Ignore spelling.

"An item which causes the spellchecker to ignore the word for this session."

> Source/WebKit2/UIProcess/API/gtk/WebKitContextMenuActions.h:57
> + * @WEBKIT_CONTEXT_MENU_ACTION_LEARN_SPELLING: Learn spelling.

'An item which causes the spellchecker to add the word to the dictionary."

> Source/WebKit2/UIProcess/API/gtk/WebKitContextMenuActions.h:59
> + * @WEBKIT_CONTEXT_MENU_ACTION_FONT_MENU: Font menu.

Not sure what this is either...

> Source/WebKit2/UIProcess/API/gtk/WebKitContextMenuActions.h:125
> +    WEBKIT_CONTEXT_MENU_ACTION_BASE_APPLICATION = 10000

WEBKIT_CONTEXT_MENU_ACTION_CUSTOM perhaps?

> Source/WebKit2/UIProcess/API/gtk/WebKitContextMenuItem.cpp:45
> +G_DEFINE_TYPE(WebKitContextMenuItem, webkit_context_menu_item, G_TYPE_INITIALLY_UNOWNED)

I like the use of G_TYPE_INITIALLY_UNOWNED here.

> Source/WebKit2/UIProcess/API/gtk/WebKitContextMenuItem.cpp:161
> + * action to be performed.

action from being performed.

> Source/WebKit2/UIProcess/API/gtk/WebKitContextMenuItem.cpp:165
> +WebKitContextMenuItem* webkit_context_menu_item_new_from_stock(WebKitContextMenuAction action)

webkit_context_menu_item_new_from_webkit_action? The use of the word stock here makes me think of the set of GTK+ stock items.

> Source/WebKit2/UIProcess/API/gtk/WebKitContextMenuItem.cpp:251
> + * webkit_context_menu_item_get_stock_action:

webkit_context_menu_item_get_webkit_action?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:964
> +     *  and return %TRUE so tha the proposed menu will not be shown.

so tha -> so that

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:970
> +     *  build your own #GtkMenu and return %TRUE to prevent the proposed menu to be shown.

prevent the proposed menu to be shown -> prevent the proposed menu from being shown

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1262
> +        if (webkit_context_menu_item_get_stock_action(item) == WEBKIT_CONTEXT_MENU_ACTION_UNICODE) {
> +            unicodeMenuItemPosition = i;
> +            break;
> +        }

It makes sense to just do return i here.

> Source/WebKit2/UIProcess/API/gtk/tests/TestContextMenu.cpp:41
> +    static gboolean viewContextMenuCallback(WebKitWebView* webView, WebKitContextMenu* contextMenu, GdkEvent* event, WebKitHitTestResult* hitTestResult, ContextMenuTest* test)

contextMenuCallback?

> Source/WebKit2/UIProcess/API/gtk/tests/TestContextMenu.cpp:46
> +        g_assert(WEBKIT_IS_CONTEXT_MENU(contextMenu));
> +        test->assertObjectIsDeletedWhenTestFinishes(G_OBJECT(contextMenu));
> +        test->checkContextMenuEvent(event);
> +        g_assert(WEBKIT_IS_HIT_TEST_RESULT(hitTestResult));

It's probably better to put these actions in the base class implementation of ::contextMenu and then just call ContextMenuTest::contextMenu from each implementation.

> Source/WebKit2/UIProcess/API/gtk/tests/TestContextMenu.cpp:49
> +        return test->contextMenu(contextMenu, event, hitTestResult);

The base class is responsible for starting the main loop, so why not quit the main loop in ContextMenuTest::contextMenu?

> Source/WebKit2/UIProcess/API/gtk/tests/TestContextMenu.cpp:83
> +        if (state & Visible)
> +            g_assert(gtk_action_get_visible(action));
> +        else
> +            g_assert(!gtk_action_get_visible(action));
> +

Perhaps this could just be g_assert(gtk_action_get_visible(action) == (state & Visible));

> Source/WebKit2/UIProcess/API/gtk/tests/TestContextMenu.cpp:94
> +        if (state & Enabled)
> +            g_assert(gtk_action_get_sensitive(action));
> +        else
> +            g_assert(!gtk_action_get_sensitive(action));
> +
> +        if (GTK_IS_TOGGLE_ACTION(action)) {
> +            if (state & Checked)
> +                g_assert(gtk_toggle_action_get_active(GTK_TOGGLE_ACTION(action)));
> +            else
> +                g_assert(!gtk_toggle_action_get_active(GTK_TOGGLE_ACTION(action)));
> +        }

Ditto.

> Source/WebKit2/UIProcess/API/gtk/tests/TestContextMenu.cpp:167
> +    void showContextMenuAtPositionAndWaitUntilFinish(int x, int y)

showContextMenuAtPositionAndWaitUntilFinish -> showContextMenuAtPositionAndWaitUntilFinished

> Source/WebKit2/UIProcess/API/gtk/tests/TestContextMenu.cpp:175
> +    void showContextMenuAndWaitUntilFinish()

Ditto.

> Source/WebKit2/UIProcess/API/gtk/tests/TestContextMenu.cpp:581
> +            finishMainLoopAfterDelay(1);

A one second delay in a test is way too long. How about just spining the main loop until there are no more events to process?

> Source/WebKit2/UIProcess/API/gtk/tests/WebViewTest.cpp:117
>      g_timeout_add_seconds(seconds, reinterpret_cast<GSourceFunc>(testLoadTimeoutFinishLoop), m_mainLoop);

Assuming spinning the main loop doesn't work above, this is a good opportunity to rename testLoadTimeoutFinishLoop to quitMainLoop and bring it closer to here.

[Carlos Garcia Campos]

(In reply to comment #5)
> (From update of attachment 147596 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=147596&action=review

Thank you very much for reviewing it ;-)

> This patch looks pretty good, but I think I'd prefer it WebKitContextMenuItem didn't contain a "live" submenu element (just a cache of the realized version of the ContextMenuItem).
> 
> The reason I say this is because I'm not sure that it makes sense to have further modifications of a submenu alter some previously instantiated WebKitContextMenuItem. Instead, it makes sense to me for the code to duplicate the submenu item's contents and not hold on to a reference. This allows code like:
> 
> WebKitContentMenuItem* item1= webkit_context_menu_item_new_with_submenu(submenu);
> 
> /* make some minor changes to the submenu */
> WebKitContentMenuItem* item2 = webkit_context_menu_item_new_with_submenu(submenu);
> 
> /* make some minor changes to the submenu */
> WebKitContentMenuItem* item3 = webkit_context_menu_item_new_with_submenu(submenu);

This is pretty weird. I've tried to make the API as similar as GTK+ as possible. In GTK+ the same menu can't be a submenu of more than one menu item, the same way a GtkWidget can't be added to more than one container. I'll add the check to make sure it doesn't happen, the same way GTK+ does.

> > Source/WebKit2/UIProcess/API/gtk/WebKitContextMenu.cpp:106
> > +WebKitContextMenu* webkit_context_menu_with_items(GList* items)
> 
> Probably best to call this webkit_context_new_menu_with_items to make it clear that it's a constructor. This seems to be the style in the Gnome platform (just searched devhelp for "new_with").

Indeed, and that was my intention. I guess I typed it wrong the first time and then I copy-pasted every time I used it :-P

> > Source/WebKit2/UIProcess/API/gtk/WebKitContextMenu.cpp:118
> > +    g_object_ref_sink(item);
> > +    menu->priv->items = g_list_insert(menu->priv->items, item, position);
> 
> I think it just makes sense to roll this into webkit_context_menu_insert. I believe the double cast/checks for webkit_context_menu_prepend and webkit_context_menu_append shouldn't be a big deal, but you can always just eliminate them in the outer methods.

Ok.

> > Source/WebKit2/UIProcess/API/gtk/WebKitContextMenu.cpp:126
> > + * Adds @item at the beginning of the @menu item list.
> 
> Probably just best to day "Adds @item at the beginning of the @menu." as the fact that it's a list is somewhat of an implementation detail. :)

I agree.

> > Source/WebKit2/UIProcess/API/gtk/WebKitContextMenu.cpp:177
> > + * Moves @item to the given position in the @menu item list.
> > + * The first position is 0.
> 
> I love how you specified the what happens when you pass weird values to webkit_context_menu_insert. Do you mind doing the same here?

Sure.

> > Source/WebKit2/UIProcess/API/gtk/WebKitContextMenuActions.h:59
> > + * @WEBKIT_CONTEXT_MENU_ACTION_FONT_MENU: Font menu.
> 
> Not sure what this is either...

me neither, I guess it's mac specific, I'll look at it again.
 
> > Source/WebKit2/UIProcess/API/gtk/WebKitContextMenuActions.h:125
> > +    WEBKIT_CONTEXT_MENU_ACTION_BASE_APPLICATION = 10000
> 
> WEBKIT_CONTEXT_MENU_ACTION_CUSTOM perhaps?

Ok, I tried to use the same values than WebCore, but I prefer CUSTOM too.

> > Source/WebKit2/UIProcess/API/gtk/WebKitContextMenuItem.cpp:165
> > +WebKitContextMenuItem* webkit_context_menu_item_new_from_stock(WebKitContextMenuAction action)
> 
> webkit_context_menu_item_new_from_webkit_action? The use of the word stock here makes me think of the set of GTK+ stock items.

actions refers to GtkAction, GTK+ has stock icons, these are webkit stock actions, it has nothing to do. We could use webkit_context_menu_item_new_from_stock_action() so that api dones't look like GTK+ api that takes a stock icon and it's consistent with webkit_context_menu_item_get_stock_action().

> > Source/WebKit2/UIProcess/API/gtk/WebKitContextMenuItem.cpp:251
> > + * webkit_context_menu_item_get_stock_action:
> 
> webkit_context_menu_item_get_webkit_action?

I prefer stock_action.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1262
> > +        if (webkit_context_menu_item_get_stock_action(item) == WEBKIT_CONTEXT_MENU_ACTION_UNICODE) {
> > +            unicodeMenuItemPosition = i;
> > +            break;
> > +        }
> 
> It makes sense to just do return i here.

Yes.

> > Source/WebKit2/UIProcess/API/gtk/tests/TestContextMenu.cpp:46
> > +        g_assert(WEBKIT_IS_CONTEXT_MENU(contextMenu));
> > +        test->assertObjectIsDeletedWhenTestFinishes(G_OBJECT(contextMenu));
> > +        test->checkContextMenuEvent(event);
> > +        g_assert(WEBKIT_IS_HIT_TEST_RESULT(hitTestResult));
> 
> It's probably better to put these actions in the base class implementation of ::contextMenu and then just call ContextMenuTest::contextMenu from each implementation.

Why is that better?, it requires to manually call the parent method from each implementation, which is easier to forget.

> > Source/WebKit2/UIProcess/API/gtk/tests/TestContextMenu.cpp:49
> > +        return test->contextMenu(contextMenu, event, hitTestResult);
> 
> The base class is responsible for starting the main loop, so why not quit the main loop in ContextMenuTest::contextMenu?

Because it's pure virtual, but I can make it just virtual and finish the mainloop there, all implementation will have explicitly call the parent class anyway, so it's even more code for the same result. Calling contextMenu() in the impl look weird to me, so I'll add a helper method in the parent to finish the loop.

> > Source/WebKit2/UIProcess/API/gtk/tests/TestContextMenu.cpp:83
> > +        if (state & Visible)
> > +            g_assert(gtk_action_get_visible(action));
> > +        else
> > +            g_assert(!gtk_action_get_visible(action));
> > +
> 
> Perhaps this could just be g_assert(gtk_action_get_visible(action) == (state & Visible));

I'll try that way.


> > Source/WebKit2/UIProcess/API/gtk/tests/TestContextMenu.cpp:581
> > +            finishMainLoopAfterDelay(1);
> 
> A one second delay in a test is way too long. How about just spining the main loop until there are no more events to process?

Ok, I'll try

> > Source/WebKit2/UIProcess/API/gtk/tests/WebViewTest.cpp:117
> >      g_timeout_add_seconds(seconds, reinterpret_cast<GSourceFunc>(testLoadTimeoutFinishLoop), m_mainLoop);
> 
> Assuming spinning the main loop doesn't work above, this is a good opportunity to rename testLoadTimeoutFinishLoop to quitMainLoop and bring it closer to here.

It makes sense.

[Carlos Garcia Campos]

(In reply to comment #5)
> (From update of attachment 147596 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=147596&action=review
> 

> > Source/WebKit2/UIProcess/API/gtk/tests/TestContextMenu.cpp:83
> > +        if (state & Visible)
> > +            g_assert(gtk_action_get_visible(action));
> > +        else
> > +            g_assert(!gtk_action_get_visible(action));
> > +
> 
> Perhaps this could just be g_assert(gtk_action_get_visible(action) == (state & Visible));

It gives a compile warning because it's a comparison between signed and unsigned.

[Carlos Garcia Campos]

Created attachment 148319 [details]
Updated patch addressing review comments

Patch updated to address review comments. I've also added a new test case for the submenu api.

[Martin Robinson]

Comment on attachment 148319 [details]
Updated patch addressing review comments

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

> Source/WebKit2/UIProcess/API/gtk/tests/TestContextMenu.cpp:562
> +    static gboolean viewButtonPressEventCallback(GtkWidget*, GdkEvent* event, ContextMenuDisabledTest* test)

buttonPressEventCallback?

[Carlos Garcia Campos]

Committed r121093: <http://trac.webkit.org/changeset/121093>