[webkit-reviews] review granted: [Bug 90449] [GTK] Implement smart separators for context menu in WebKit2 GTK+ : [Attachment 150587] Patch

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has granted Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 90449: [GTK] Implement smart separators for context menu in WebKit2 GTK+
https://bugs.webkit.org/show_bug.cgi?id=90449

Attachment 150587: Patch
https://bugs.webkit.org/attachment.cgi?id=150587&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
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!