[webkit-reviews] review denied: [Bug 81011] [GTK] Add ContextMenu API to WebKit2 GTK+ API : [Attachment 147596] Patch updated to apply on current git master
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Jun 18 10:07:46 PDT 2012
Martin Robinson <mrobinson at webkit.org> has denied Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 81011: [GTK] Add ContextMenu API to WebKit2 GTK+ API
https://bugs.webkit.org/show_bug.cgi?id=81011
Attachment 147596: Patch updated to apply on current git master
https://bugs.webkit.org/attachment.cgi?id=147596&action=review
------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
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.
More information about the webkit-reviews
mailing list