[Webkit-unassigned] [Bug 81011] [GTK] Add ContextMenu API to WebKit2 GTK+ API

Tue Jun 19 01:07:36 PDT 2012

https://bugs.webkit.org/show_bug.cgi?id=81011

--- Comment #6 from Carlos Garcia Campos <cgarcia at igalia.com>  2012-06-19 01:07:35 PST ---
(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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.