Bug 90449 – [GTK] Implement smart separators for context menu in WebKit2 GTK+

• Component: WebKit2
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

Created attachment 150587 [details]
Patch

[Xan Lopez]

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?

[Carlos Garcia Campos]

(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.

[Martin Robinson]

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!

[Carlos Garcia Campos]

(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

[Carlos Garcia Campos]

Committed r125528: <http://trac.webkit.org/changeset/125528>