With the context menu api exposed in WebKit2 GTK+ now it's possible that the context menu shows separators at the beginning or end of the menu or two separators together due to an action hidden after the context menu is created.
Created attachment 150587 [details] Patch
Comment on attachment 150587 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=150587&action=review > Source/WebKit2/UIProcess/gtk/WebContextMenuProxyGtk.cpp:123 > + } It seems weird that WK would ask us to populate menus with separators in stupid places. If this is the case should we not fix that code instead?
(In reply to comment #2) > (From update of attachment 150587 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=150587&action=review > > > Source/WebKit2/UIProcess/gtk/WebContextMenuProxyGtk.cpp:123 > > + } > > It seems weird that WK would ask us to populate menus with separators in stupid places. If this is the case should we not fix that code instead? This is not called from WK, but from the API layer. The other populate method is the one used for WK menu items, and it doesn't check separators.
Comment on attachment 150587 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=150587&action=review Okay. A couple small suggestions follow... > Source/WebKit2/UIProcess/API/gtk/tests/TestContextMenu.cpp:752 > + void contextMenuDismissed() > + { > + ContextMenuTest::contextMenuDismissed(); > + } Hrm. Why override the parent method only to simply call the parent method? > Source/WebKit2/UIProcess/gtk/WebContextMenuProxyGtk.cpp:63 > + bool previousNonSeparatorItemIsVisible = false; This might be better named: previousVisibleItemIsNotASeparator > Source/WebKit2/UIProcess/gtk/WebContextMenuProxyGtk.cpp:64 > + GtkWidget* lastItemVisibleSeparator = 0; For clarity sake (though it's slightly more expensive), perhaps you could name this lastVisibleItem and then below just do: if (GTK_IS_SEPARATOR_MENU_ITEM(lastItemVisibleSeparator)) gtk_widget_hide(lastVisibleItem); You could also avoid setting this to null all the time when dealing with non-separator items. > Source/WebKit2/UIProcess/gtk/WebContextMenuProxyGtk.cpp:68 > + if (GTK_IS_SEPARATOR_MENU_ITEM(iter->data)) { Could you use widget here instead of iter->data? > Source/WebKit2/UIProcess/gtk/WebContextMenuProxyGtk.cpp:69 > + if (previousNonSeparatorItemIsVisible) { Maybe you could even replace this check with: if (lastVisibleItem && !GTK_IS_SEPARTOR(lastVisibleItem)) and then get rid of previousNonSeparatorItemIsVisible entirely. I think that might make this loop a bit easier to read. > Source/WebKit2/UIProcess/gtk/WebContextMenuProxyGtk.cpp:122 > + for (size_t i = 0; i < items.size(); i++) { > + ContextMenuItem& menuItem = items.at(i); > + if (menuItem.type() == SeparatorType) { > + previousIsSeparator = true; > + continue; > + } > + > + if (previousIsSeparator && !isEmpty) > + append(items.at(i - 1)); > + previousIsSeparator = false; > + > + append(menuItem); > + isEmpty = false; This is a bit tricky, so I think it deserves a comment. It's not clear from glancing at this code that it prevents all of these situations: 1. Separators next to each other. 2. Separators at the beginning of the menu. 3. Separators at the end of the menu. The third one is quite subtle!
(In reply to comment #4) > (From update of attachment 150587 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=150587&action=review > > Okay. A couple small suggestions follow... > > > Source/WebKit2/UIProcess/API/gtk/tests/TestContextMenu.cpp:752 > > + void contextMenuDismissed() > > + { > > + ContextMenuTest::contextMenuDismissed(); > > + } > > Hrm. Why override the parent method only to simply call the parent method? Good point, I don't know :-P > > Source/WebKit2/UIProcess/gtk/WebContextMenuProxyGtk.cpp:63 > > + bool previousNonSeparatorItemIsVisible = false; > > This might be better named: previousVisibleItemIsNotASeparator Ok. > > Source/WebKit2/UIProcess/gtk/WebContextMenuProxyGtk.cpp:64 > > + GtkWidget* lastItemVisibleSeparator = 0; > > For clarity sake (though it's slightly more expensive), perhaps you could name this lastVisibleItem and then below just do: > > if (GTK_IS_SEPARATOR_MENU_ITEM(lastItemVisibleSeparator)) > gtk_widget_hide(lastVisibleItem); We have already checked whether every item is a separator or not. > You could also avoid setting this to null all the time when dealing with non-separator items. But I would have to set it to the widget in any case. > > Source/WebKit2/UIProcess/gtk/WebContextMenuProxyGtk.cpp:68 > > + if (GTK_IS_SEPARATOR_MENU_ITEM(iter->data)) { > > Could you use widget here instead of iter->data? Yes, indeed. > > Source/WebKit2/UIProcess/gtk/WebContextMenuProxyGtk.cpp:69 > > + if (previousNonSeparatorItemIsVisible) { > > Maybe you could even replace this check with: > > if (lastVisibleItem && !GTK_IS_SEPARTOR(lastVisibleItem)) > > and then get rid of previousNonSeparatorItemIsVisible entirely. I think that might make this loop a bit easier to read. I don't think it's easier to read because you need to check all the time (and again) whether the last visible item is a separator or not. > > Source/WebKit2/UIProcess/gtk/WebContextMenuProxyGtk.cpp:122 > > + for (size_t i = 0; i < items.size(); i++) { > > + ContextMenuItem& menuItem = items.at(i); > > + if (menuItem.type() == SeparatorType) { > > + previousIsSeparator = true; > > + continue; > > + } > > + > > + if (previousIsSeparator && !isEmpty) > > + append(items.at(i - 1)); > > + previousIsSeparator = false; > > + > > + append(menuItem); > > + isEmpty = false; > > This is a bit tricky, so I think it deserves a comment. fair enough > It's not clear from glancing at this code that it prevents all of these situations: > 1. Separators next to each other. > 2. Separators at the beginning of the menu. > 3. Separators at the end of the menu. > > The third one is quite subtle! Ok, I'll add a comment
Committed r125528: <http://trac.webkit.org/changeset/125528>