WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
90449
[GTK] Implement smart separators for context menu in WebKit2 GTK+
https://bugs.webkit.org/show_bug.cgi?id=90449
Summary
[GTK] Implement smart separators for context menu in WebKit2 GTK+
Carlos Garcia Campos
Reported
2012-07-03 05:09:26 PDT
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.
Attachments
Patch
(13.39 KB, patch)
2012-07-03 05:15 PDT
,
Carlos Garcia Campos
mrobinson
: review+
mrobinson
: commit-queue-
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Carlos Garcia Campos
Comment 1
2012-07-03 05:15:05 PDT
Created
attachment 150587
[details]
Patch
Xan Lopez
Comment 2
2012-07-13 05:11:12 PDT
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
Comment 3
2012-07-13 05:15:00 PDT
(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 4
2012-08-13 02:45:59 PDT
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
Comment 5
2012-08-13 23:53:51 PDT
(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
Comment 6
2012-08-14 02:06:41 PDT
Committed
r125528
: <
http://trac.webkit.org/changeset/125528
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug