[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





--- Comment #5 from Carlos Garcia Campos <cgarcia at igalia.com>  2012-08-13 23:53:51 PST ---
(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

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