A new menu system.

A new menu system for views.

This is all the functionality needed for the page, app menus and browser system menus.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=17895

[sky, 2009-06-05 15:54:37]

This all sounds reasonable to me. The only substantial comment I have is that I
tend to think it will be easier for consumers not to have to create child
Menu2Models for any nested menus. If you pass in an id of the parent (0 for the
root) to all the methods than you don't need to create nested Menu2Models.

http://codereview.chromium.org/119237/diff/1001/1002
File views/controls/menu/menu_2.h (right):

http://codereview.chromium.org/119237/diff/1001/1002#newcode53
Line 53: virtual int GetGroupIDAt(int index) const = 0;
nit: ID -> Id (for consistency with GetCommandIdAt (or name both ID).

[sky, 2009-06-08 16:41:34]

Looks like you forgot to include controls/menu/menu_2.cc in this patch.

http://codereview.chromium.org/119237/diff/188/198
File base/gfx/native_widget_types.h (right):

http://codereview.chromium.org/119237/diff/188/198#newcode78
Line 78: typedef GtkWidget* NativeMenu;
I'm not intimately familiar with GTK menus, but they do have a couple of menu
related classes, such as GtkMenu. You sure you don't want that?

http://codereview.chromium.org/119237/diff/188/193
File views/controls/menu/native_menu_gtk.cc (right):

http://codereview.chromium.org/119237/diff/188/193#newcode14
Line 14: struct UpdateStateData {
Description?

http://codereview.chromium.org/119237/diff/188/193#newcode19
Line 19: struct Position {
Description?

http://codereview.chromium.org/119237/diff/188/193#newcode136
Line 136: 
What about the icon? Perhaps a todo for now.

http://codereview.chromium.org/119237/diff/188/193#newcode140
Line 140: Menu2* submenu = new Menu2(model_->GetSubmenuModelAt(index),
delegate_);
ScopedVector maybe?

http://codereview.chromium.org/119237/diff/188/193#newcode168
Line 168: if (gtk_menu_item_get_submenu(GTK_MENU_ITEM(menu_item))) {
Doesn't this return the menu, so that you don't need "submenu"?

http://codereview.chromium.org/119237/diff/188/193#newcode173
Line 173: submenu->UpdateStates();
gtk_container_foreach doesn't recurse for you? How lame!

http://codereview.chromium.org/119237/diff/188/195
File views/controls/menu/native_menu_win.cc (right):

http://codereview.chromium.org/119237/diff/188/195#newcode32
Line 32: L"", 0, 0, 0, 0, 0, HWND_MESSAGE, NULL, NULL, NULL);
nit: L"" -> std::wstring()

http://codereview.chromium.org/119237/diff/188/195#newcode35
Line 35: ~MenuHostWindow() {
nit: newline before ~.

http://codereview.chromium.org/119237/diff/188/195#newcode135
Line 135: owner_draw_ = model_->HasIcons();
This will break the icons in the back/forward drop down. Perhaps the right thing
to do is to have BackForwardMenuModel override HasIcons to return true.

http://codereview.chromium.org/119237/diff/188/195#newcode266
Line 266: items_.at(model_index)->label = formatted;
items_.at -> items_[]

http://codereview.chromium.org/119237/diff/188/196
File views/controls/menu/native_menu_win.h (right):

http://codereview.chromium.org/119237/diff/188/196#newcode51
Line 51: bool IsSeparatorItemAt(int menu_index) const;
Did you consider having all these only take a menu_index (or model_index)? As
you can always calculate one from the other, you don't really need to pass in
both. Seems like it might be less error prone with just one and it's more
consistent.

[Ben Goodger (Google), 2009-06-08 18:54:48]

updated and added comments...

http://codereview.chromium.org/119237/diff/188/198
File base/gfx/native_widget_types.h (right):

http://codereview.chromium.org/119237/diff/188/198#newcode78
Line 78: typedef GtkWidget* NativeMenu;
Nope. Most Gtk code seems to assume GtkWidget - in fact, that's what
gtk_menu_new() returns, you can then cast to GtkMenu using GTK_IS_MENU() and
GTK_MENU()

http://codereview.chromium.org/119237/diff/188/193
File views/controls/menu/native_menu_gtk.cc (right):

http://codereview.chromium.org/119237/diff/188/193#newcode140
Line 140: Menu2* submenu = new Menu2(model_->GetSubmenuModelAt(index),
delegate_);
On 2009/06/08 16:41:34, sky wrote:
> ScopedVector maybe?

The problem is destroying the Menu2 object will destroy the wrapper, which is
another instance of this class, which will delete the nested GtkMenu... but I
think the GtkMenu is actually owned by the GtkWidget hierarchy... so we could
get in a bad state.

In a future iteration I think instead of creating a new Menu2 I'm going to stash
a pointer to the Menu2Model on the item and rejigger Rebuild/Update a little to
obtain that. Menu2Model is owned by someone else so we don't need to worry about
cleaning it up.

http://codereview.chromium.org/119237/diff/188/193#newcode168
Line 168: if (gtk_menu_item_get_submenu(GTK_MENU_ITEM(menu_item))) {
On 2009/06/08 16:41:34, sky wrote:
> Doesn't this return the menu, so that you don't need "submenu"?

gtk_menu_item_get_submenu returns non-NULL if the menu_item has a submenu.
That's all the information I need... maybe I'm not understanding your question?

http://codereview.chromium.org/119237/diff/188/193#newcode173
Line 173: submenu->UpdateStates();
On 2009/06/08 16:41:34, sky wrote:
> gtk_container_foreach doesn't recurse for you? How lame!

I don't know - I suspect not, since it's operating on a child list directly
associated with a widget. At this point it's probably for the best since this
way we get the correct model for free.

http://codereview.chromium.org/119237/diff/188/195
File views/controls/menu/native_menu_win.cc (right):

http://codereview.chromium.org/119237/diff/188/195#newcode135
Line 135: owner_draw_ = model_->HasIcons();
The Back/Forward menu model isn't using this code yet...

http://codereview.chromium.org/119237/diff/188/196
File views/controls/menu/native_menu_win.h (right):

http://codereview.chromium.org/119237/diff/188/196#newcode51
Line 51: bool IsSeparatorItemAt(int menu_index) const;
Does it make it less error prone, or does it just shift responsibility for
calculating the values to many different places? It seems having two index
spaces is the most error prone aspect ;-)

[sky, 2009-06-08 19:30:23]

LGTM with the following changes.

http://codereview.chromium.org/119237/diff/188/193
File views/controls/menu/native_menu_gtk.cc (right):

http://codereview.chromium.org/119237/diff/188/193#newcode168
Line 168: if (gtk_menu_item_get_submenu(GTK_MENU_ITEM(menu_item))) {
On 2009/06/08 18:54:48, Ben Goodger wrote:
> On 2009/06/08 16:41:34, sky wrote:
> > Doesn't this return the menu, so that you don't need "submenu"?
> 
> gtk_menu_item_get_submenu returns non-NULL if the menu_item has a submenu.
> That's all the information I need... maybe I'm not understanding your
question?

I'm stupid here, nevermind.

http://codereview.chromium.org/119237/diff/210/1213
File views/controls/menu/menu_2.h (right):

http://codereview.chromium.org/119237/diff/210/1213#newcode88
Line 88: virtual Menu2Model* GetSubmenuModelAt(int index) const = 0;
I think it's worth mentioning ownership here.

http://codereview.chromium.org/119237/diff/210/1213#newcode141
Line 141: MenuWrapper* wrapper_;
It looks like this gets leaked. Should this be scoped_ptr?