[Webkit-unassigned] [Bug 90449] [GTK] Implement smart separators for context menu in WebKit2 GTK+

[bugzilla-daemon at webkit.org]

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


Martin Robinson <mrobinson at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #150587|review?                     |review+, commit-queue-
               Flag|                            |




--- Comment #4 from Martin Robinson <mrobinson at webkit.org>  2012-08-13 02:45:59 PST ---
(From update of attachment 150587)
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!

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