GTK: First draft of the unified cut/copy/paste and +/-/Fullscreen menu items.

GTK: First draft of the unified cut/copy/paste and +/-/Fullscreen menu items.

Adds special menu item types that allow shoving buttons into them, along with
tracking which button is selected. We now are halfway to the mocks that the
chrome-ui-leads sent out.

BUG=45757
TEST=none

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

[sky, 2010-06-18 20:48:35]

I'm planning on taking a different approach on windows:
http://codereview.chromium.org/2803013/show . That's not to say I don't like
what you've done, just that it's easier for me to go the route I did for how I
need to wire this all up.

I assume you're going to have Evan review the Gtk side, so the rest LGTM.

  -Scott

http://codereview.chromium.org/2800015/diff/2001/3001
File app/menus/menu_model.h (right):

http://codereview.chromium.org/2800015/diff/2001/3001#newcode89
app/menus/menu_model.h:89: // Get the native view that does in the menu at the
specified index (or NULL
does->goes

http://codereview.chromium.org/2800015/diff/2001/3002
File app/menus/simple_menu_model.cc (right):

http://codereview.chromium.org/2800015/diff/2001/3002#newcode210
app/menus/simple_menu_model.cc:210: return items_.at(index).item;
items_[index]

http://codereview.chromium.org/2800015/diff/2001/3002#newcode215
app/menus/simple_menu_model.cc:215: if (!delegate_ || command_id == kSeparatorId
|| items_.at(index).item)
items_[index]

http://codereview.chromium.org/2800015/diff/2001/3003
File app/menus/simple_menu_model.h (right):

http://codereview.chromium.org/2800015/diff/2001/3003#newcode130
app/menus/simple_menu_model.h:130: gfx::NativeMenuItem item;
Description?

http://codereview.chromium.org/2800015/diff/2001/3009
File chrome/browser/gtk/gtk_custom_menu.cc (right):

http://codereview.chromium.org/2800015/diff/2001/3009#newcode11
chrome/browser/gtk/gtk_custom_menu.cc:11: // Stolen directly from
gtkmenushell.c. I'd love to call the library version
Do you need to copy a license header if you're copying code?

http://codereview.chromium.org/2800015/diff/2001/3012
File chrome/browser/gtk/gtk_custom_menu_item.h (right):

http://codereview.chromium.org/2800015/diff/2001/3012#newcode22
chrome/browser/gtk/gtk_custom_menu_item.h:22: // selected and otherwise . It
otherwise acts like a normal GtkItem. The
and otherwise?

[Elliot Glaysher, 2010-06-18 22:59:47]

+evan for sky's comment on licensing.

http://codereview.chromium.org/2800015/diff/2001/3002
File app/menus/simple_menu_model.cc (right):

http://codereview.chromium.org/2800015/diff/2001/3002#newcode210
app/menus/simple_menu_model.cc:210: return items_.at(index).item;
On 2010/06/18 20:48:36, sky wrote:
> items_[index]

I'm matching every other invocation of .at() in the class.

http://codereview.chromium.org/2800015/diff/2001/3009
File chrome/browser/gtk/gtk_custom_menu.cc (right):

http://codereview.chromium.org/2800015/diff/2001/3009#newcode11
chrome/browser/gtk/gtk_custom_menu.cc:11: // Stolen directly from
gtkmenushell.c. I'd love to call the library version
On 2010/06/18 20:48:36, sky wrote:
> Do you need to copy a license header if you're copying code?

Probably not. There's a lot more copied/modified gtk functions in the chrome
linux port. Adding evanm for an opinion.

[sky, 2010-06-18 23:03:52]

http://codereview.chromium.org/2800015/diff/2001/3002
File app/menus/simple_menu_model.cc (right):

http://codereview.chromium.org/2800015/diff/2001/3002#newcode210
app/menus/simple_menu_model.cc:210: return items_.at(index).item;
On 2010/06/18 22:59:47, Elliot Glaysher wrote:
> On 2010/06/18 20:48:36, sky wrote:
> > items_[index]
> 
> I'm matching every other invocation of .at() in the class.

Looks like the class is inconsistent:( GetIconAt doesn't use at.

[Evan Stade, 2010-06-18 23:44:32]

app/menus/menu_model.h:35: TYPE_NATIVE,

this totally breaks the abstraction layering. Isn't it possible to define a new
type that is still model-y? Called something like TYPE_BUTTON. Then when you
define n TYPE_BUTTONs in a row, the view knows to combine them.

[Elliot Glaysher, 2010-06-21 23:21:17]

On 2010/06/18 23:44:32, Evan Stade wrote:
> app/menus/menu_model.h:35: TYPE_NATIVE,
> 
> this totally breaks the abstraction layering. Isn't it possible to define a
new
> type that is still model-y? Called something like TYPE_BUTTON. Then when you
> define n TYPE_BUTTONs in a row, the view knows to combine them.

Not really. While in this first draft, all of them are just buttons, the
mocks[1] show a button with an icon, a faded label showing the zoom percentage,
and weird compound buttons. Now that sky says he's not going to use any of that,
I might as well just tear this out

Sky said I should try a MenuGtk subclass that removes the normal menu items
after creation and insert the new ones. I'm going to try that approach next.

[1] folder/alcor/p/Chrome_-_Menu/Menu.png

[Elliot Glaysher, 2010-06-22 19:39:55]

On 2010/06/18 23:44:32, Evan Stade wrote:
> app/menus/menu_model.h:35: TYPE_NATIVE,
> 
> this totally breaks the abstraction layering. Isn't it possible to define a
new
> type that is still model-y? Called something like TYPE_BUTTON. Then when you
> define n TYPE_BUTTONs in a row, the view knows to combine them.

So after trying it the way sky suggested (and watching it get unacceptably hacky
really fast), I'm back to the solution I submitted.

Would you still prefer a TYPE_BUTTON solution if I added six different button
types?

[Evan Stade, 2010-06-22 19:48:13]

On 2010/06/22 19:39:55, Elliot Glaysher wrote:
> On 2010/06/18 23:44:32, Evan Stade wrote:
> > app/menus/menu_model.h:35: TYPE_NATIVE,
> > 
> > this totally breaks the abstraction layering. Isn't it possible to define a
> new
> > type that is still model-y? Called something like TYPE_BUTTON. Then when you
> > define n TYPE_BUTTONs in a row, the view knows to combine them.
> 
> So after trying it the way sky suggested (and watching it get unacceptably
hacky
> really fast), I'm back to the solution I submitted.
> 
> Would you still prefer a TYPE_BUTTON solution if I added six different button
> types?

why do you need 6 different types? I would prefer a TYPE_BUTTON + getters
solution (similar to TYPE_COMMAND + GetLabelAt, GetIconAt, etc) for whatever
additional information is needed. Maybe even creating a ButtonMenuItemModel
would be profitable.

[Elliot Glaysher, 2010-06-23 01:28:54]

On 2010/06/22 19:48:13, Evan Stade wrote:
> On 2010/06/22 19:39:55, Elliot Glaysher wrote:
> > On 2010/06/18 23:44:32, Evan Stade wrote:
> > > app/menus/menu_model.h:35: TYPE_NATIVE,
> > > 
> > > this totally breaks the abstraction layering. Isn't it possible to define
a
> > new
> > > type that is still model-y? Called something like TYPE_BUTTON. Then when
you
> > > define n TYPE_BUTTONs in a row, the view knows to combine them.
> > 
> > So after trying it the way sky suggested (and watching it get unacceptably
> hacky
> > really fast), I'm back to the solution I submitted.
> > 
> > Would you still prefer a TYPE_BUTTON solution if I added six different
button
> > types?
> 
> why do you need 6 different types? I would prefer a TYPE_BUTTON + getters
> solution (similar to TYPE_COMMAND + GetLabelAt, GetIconAt, etc) for whatever
> additional information is needed. Maybe even creating a ButtonMenuItemModel
> would be profitable.

Overhauled. Now uses a ButtonMenuItemModel (still needs more additions, but this
is probably a stable committing point).

[sky, 2010-06-23 04:05:56]

http://codereview.chromium.org/2800015/diff/23001/24002
File app/menus/button_menu_item_model.cc (right):

http://codereview.chromium.org/2800015/diff/23001/24002#newcode41
app/menus/button_menu_item_model.cc:41: return items_.at(index).type;
Is there a reason to use at over []?

http://codereview.chromium.org/2800015/diff/23001/24003
File app/menus/button_menu_item_model.h (right):

http://codereview.chromium.org/2800015/diff/23001/24003#newcode58
app/menus/button_menu_item_model.h:58: void AddItemWithImage(int command_id, int
icon_idr);
SkBitmap

http://codereview.chromium.org/2800015/diff/23001/24003#newcode60
app/menus/button_menu_item_model.h:60: // Adds a small vertical space.
vertical -> horizontal

http://codereview.chromium.org/2800015/diff/23001/24003#newcode72
app/menus/button_menu_item_model.h:72: string16 GetLabelAt(int index) const;
const string16&

http://codereview.chromium.org/2800015/diff/23001/24003#newcode76
app/menus/button_menu_item_model.h:76: bool GetIconAt(int index, int* icon)
const;
Use SkBitmap instead of int*

http://codereview.chromium.org/2800015/diff/23001/24003#newcode85
app/menus/button_menu_item_model.h:85: string16 item_label;
item_label_

http://codereview.chromium.org/2800015/diff/23001/24004
File app/menus/menu_model.h (right):

http://codereview.chromium.org/2800015/diff/23001/24004#newcode91
app/menus/menu_model.h:91: virtual menus::ButtonMenuItemModel*
GetButtonMenuItemAt(int index) const = 0;
nit: don't need menus::

http://codereview.chromium.org/2800015/diff/23001/24005
File app/menus/simple_menu_model.cc (right):

http://codereview.chromium.org/2800015/diff/23001/24005#newcode216
app/menus/simple_menu_model.cc:216: if (!delegate_ || command_id == kSeparatorId
|| items_.at(index).button_model)
FlipIndex

http://codereview.chromium.org/2800015/diff/23001/24006
File app/menus/simple_menu_model.h (right):

http://codereview.chromium.org/2800015/diff/23001/24006#newcode64
app/menus/simple_menu_model.h:64: void AddButtonItem(int command_id,
ButtonMenuItemModel* model);
Mention ownership.

http://codereview.chromium.org/2800015/diff/23001/24021
File chrome/browser/wrench_menu_model.cc (right):

http://codereview.chromium.org/2800015/diff/23001/24021#newcode203
chrome/browser/wrench_menu_model.cc:203: IDS_ZOOM_PERCENT,
IntToString16(zoom_percent));
How are the platforms going to know to update the widgets after this change
happens?

http://codereview.chromium.org/2800015/diff/23001/24021#newcode217
chrome/browser/wrench_menu_model.cc:217: int zoom_level =
zoom_map->GetZoomLevel(selected_tab->GetURL());
Please add a comment as to where this code comes from.

http://codereview.chromium.org/2800015/diff/23001/24022
File chrome/browser/wrench_menu_model.h (right):

http://codereview.chromium.org/2800015/diff/23001/24022#newcode65
chrome/browser/wrench_menu_model.h:65: void UpdateZoomControls();
Comments?

[Elliot Glaysher, 2010-06-23 17:23:04]

Updated.

http://codereview.chromium.org/2800015/diff/23001/24002
File app/menus/button_menu_item_model.cc (right):

http://codereview.chromium.org/2800015/diff/23001/24002#newcode41
app/menus/button_menu_item_model.cc:41: return items_.at(index).type;
On 2010/06/23 04:05:56, sky wrote:
> Is there a reason to use at over []?

Error checking, but I guess I'll use [].

http://codereview.chromium.org/2800015/diff/23001/24003
File app/menus/button_menu_item_model.h (right):

http://codereview.chromium.org/2800015/diff/23001/24003#newcode58
app/menus/button_menu_item_model.h:58: void AddItemWithImage(int command_id, int
icon_idr);
On 2010/06/23 04:05:56, sky wrote:
> SkBitmap

No, this needs to be the IDR number. I originally passed SkBitmaps here and
thankfully caught this before it was baked into the interface.

The images used in menus seem to have been made with the assumption that menu
labels would always be black. On Linux, the 3rd and 5th most popular themes are
white text on a black background. This resource needs to go through the theming
architecture, and thus, needs the IDR.

http://codereview.chromium.org/2800015/diff/23001/24021
File chrome/browser/wrench_menu_model.cc (right):

http://codereview.chromium.org/2800015/diff/23001/24021#newcode203
chrome/browser/wrench_menu_model.cc:203: IDS_ZOOM_PERCENT,
IntToString16(zoom_percent));
On 2010/06/23 04:05:56, sky wrote:
> How are the platforms going to know to update the widgets after this change
> happens?

Right now, they don't know. This is a TODO; the current patch is growing
unweildly so I'm trying to get what I've written so far in a committable state.

[sky, 2010-06-23 17:57:42]

Please keep the old cut/copy/paste plus/minus/fullscreen items for
TOOLKIT_VIEWS with a TODO for me. I'll nuke it once you straighten out
the rough edges and I convert the windows code appropriately.

  -Scott

On Wed, Jun 23, 2010 at 10:23 AM,  <erg@chromium.org> wrote:
> Updated.
>
>
> http://codereview.chromium.org/2800015/diff/23001/24002
> File app/menus/button_menu_item_model.cc (right):
>
> http://codereview.chromium.org/2800015/diff/23001/24002#newcode41
> app/menus/button_menu_item_model.cc:41: return items_.at(index).type;
> On 2010/06/23 04:05:56, sky wrote:
>>
>> Is there a reason to use at over []?
>
> Error checking, but I guess I'll use [].
>
> http://codereview.chromium.org/2800015/diff/23001/24003
> File app/menus/button_menu_item_model.h (right):
>
> http://codereview.chromium.org/2800015/diff/23001/24003#newcode58
> app/menus/button_menu_item_model.h:58: void AddItemWithImage(int
> command_id, int icon_idr);
> On 2010/06/23 04:05:56, sky wrote:
>>
>> SkBitmap
>
> No, this needs to be the IDR number. I originally passed SkBitmaps here
> and thankfully caught this before it was baked into the interface.
>
> The images used in menus seem to have been made with the assumption that
> menu labels would always be black. On Linux, the 3rd and 5th most
> popular themes are white text on a black background. This resource needs
> to go through the theming architecture, and thus, needs the IDR.
>
> http://codereview.chromium.org/2800015/diff/23001/24021
> File chrome/browser/wrench_menu_model.cc (right):
>
> http://codereview.chromium.org/2800015/diff/23001/24021#newcode203
> chrome/browser/wrench_menu_model.cc:203: IDS_ZOOM_PERCENT,
> IntToString16(zoom_percent));
> On 2010/06/23 04:05:56, sky wrote:
>>
>> How are the platforms going to know to update the widgets after this
>
> change
>>
>> happens?
>
> Right now, they don't know. This is a TODO; the current patch is growing
> unweildly so I'm trying to get what I've written so far in a committable
> state.
>
> http://codereview.chromium.org/2800015/show
>

[Evan Stade, 2010-06-23 19:04:43]

http://codereview.chromium.org/2800015/diff/12026/26004
File app/menus/button_menu_item_model.h (right):

http://codereview.chromium.org/2800015/diff/12026/26004#newcode22
app/menus/button_menu_item_model.h:22: class ButtonMenuItemModel {
I wonder if this class should inherit from SimpleMenuModel? Many of the methods
(and the delegate) seem very similar.

http://codereview.chromium.org/2800015/diff/12026/26021
File chrome/browser/wrench_menu_model.cc (right):

http://codereview.chromium.org/2800015/diff/12026/26021#newcode262
chrome/browser/wrench_menu_model.cc:262: bool WrenchMenuModel::IsDynamicItem(int
index) const {
why is this not just implemented in the delegate callback?

http://codereview.chromium.org/2800015/diff/12026/26022
File chrome/browser/wrench_menu_model.h (right):

http://codereview.chromium.org/2800015/diff/12026/26022#newcode39
chrome/browser/wrench_menu_model.h:39: public
menus::ButtonMenuItemModel::Delegate,
Why is the BrowserToolbarGtk the delegate for the menu model, but not the
buttonmenuitemmodel? Seems this class should be the delegate for both the menu
model and the menu button item model.

Also I'm pretty sure this class should not inherit from SimpleMenuModel; it
should have a SimpleMenuModel member.

http://codereview.chromium.org/2800015/diff/12026/26022#newcode79
chrome/browser/wrench_menu_model.h:79: scoped_ptr<menus::ButtonMenuItemModel>
edit_menu_model_;
nit: edit_menu_item_model_ (this and the next one)

[Elliot Glaysher, 2010-06-23 19:43:24]

http://codereview.chromium.org/2800015/diff/12026/26004
File app/menus/button_menu_item_model.h (right):

http://codereview.chromium.org/2800015/diff/12026/26004#newcode22
app/menus/button_menu_item_model.h:22: class ButtonMenuItemModel {
On 2010/06/23 19:04:43, Evan Stade wrote:
> I wonder if this class should inherit from SimpleMenuModel? Many of the
methods
> (and the delegate) seem very similar.

I'm not sure if it would be better. I think there's going to be less similarity
as I add ButtonSides and grouping for sizing.

http://codereview.chromium.org/2800015/diff/12026/26022
File chrome/browser/wrench_menu_model.h (right):

http://codereview.chromium.org/2800015/diff/12026/26022#newcode39
chrome/browser/wrench_menu_model.h:39: public
menus::ButtonMenuItemModel::Delegate,
n 2010/06/23 19:04:43, Evan Stade wrote:
> Why is the BrowserToolbarGtk the delegate for the menu model, but not the
> buttonmenuitemmodel? Seems this class should be the delegate for both the menu
> model and the menu button item model.
>
> Also I'm pretty sure this class should not inherit from SimpleMenuModel; it
> should have a SimpleMenuModel member.

Diving through the commit history (http://codereview.chromium.org/523147), it
looks like this was done by the sync guys because (I assume) there would be code
repetition on the different platforms if it was left up to BrowserToolbarGtk (or
platform equivalents).

I do have a slight preference for composition over inheritance. I've filed a
cleanup bug for this (crbug.com/47320), but this would have to be done after a
decision has been made about the new wrench menu because both the old
AppMenuModel and WrenchMenuModel both go through this pathway.

[Evan Stade, 2010-06-23 20:06:48]

http://codereview.chromium.org/2800015/diff/12026/26022
File chrome/browser/wrench_menu_model.h (right):

http://codereview.chromium.org/2800015/diff/12026/26022#newcode39
chrome/browser/wrench_menu_model.h:39: public
menus::ButtonMenuItemModel::Delegate,
On 2010/06/23 19:43:24, Elliot Glaysher wrote:
> n 2010/06/23 19:04:43, Evan Stade wrote:
> > Why is the BrowserToolbarGtk the delegate for the menu model, but not the
> > buttonmenuitemmodel? Seems this class should be the delegate for both the
menu
> > model and the menu button item model.
> >
> > Also I'm pretty sure this class should not inherit from SimpleMenuModel; it
> > should have a SimpleMenuModel member.
> 
> Diving through the commit history (http://codereview.chromium.org/523147), it
> looks like this was done by the sync guys because (I assume) there would be
code
> repetition on the different platforms if it was left up to BrowserToolbarGtk
(or
> platform equivalents).
> 
> I do have a slight preference for composition over inheritance. I've filed a
> cleanup bug for this (crbug.com/47320), but this would have to be done after a
> decision has been made about the new wrench menu because both the old
> AppMenuModel and WrenchMenuModel both go through this pathway.

well the reason I bring it up now is because if we do this (and I think we
should), then there will be naming conflicts with the
ButtonMenuItemModel::Delegate (although not if that Delegate is the same as
SimpleMenuModel::Delegate)

[Elliot Glaysher, 2010-06-23 23:59:24]

On Wed, Jun 23, 2010 at 1:06 PM,  <estade@chromium.org> wrote:
> well the reason I bring it up now is because if we do this (and I think
> we should), then there will be naming conflicts with the
> ButtonMenuItemModel::Delegate (although not if that Delegate is the same
> as SimpleMenuModel::Delegate)

But the shared methods are keyed on command_id, and not position so
I'm not sure I see the problem. There isn't going to be a conflict
between (for example) indexes into the ButtonMenuItem and indexes into
the SimpleMenuModel.

> http://codereview.chromium.org/2800015/show
>

[Evan Stade, 2010-06-24 00:15:05]

http://codereview.chromium.org/2800015/diff/12026/26022
File chrome/browser/wrench_menu_model.h (right):

http://codereview.chromium.org/2800015/diff/12026/26022#newcode39
chrome/browser/wrench_menu_model.h:39: public
menus::ButtonMenuItemModel::Delegate,
I thought it would be a problem for a C++ class to have two superclasses with
the same function names?

[Elliot Glaysher, 2010-06-24 00:22:51]

On Wed, Jun 23, 2010 at 5:15 PM,  <estade@chromium.org> wrote:
>
> http://codereview.chromium.org/2800015/diff/12026/26022
> File chrome/browser/wrench_menu_model.h (right):
>
> http://codereview.chromium.org/2800015/diff/12026/26022#newcode39
> chrome/browser/wrench_menu_model.h:39: public
> menus::ButtonMenuItemModel::Delegate,
> I thought it would be a problem for a C++ class to have two superclasses
> with the same function names?

Nope. This compiles does exactly what you expect:

#include <iostream>

using namespace std;

class Interface1 {
 public:
  virtual void Do() const = 0;
};

class Interface2 {
 public:
  virtual void Do() const = 0;
};

class Does : public Interface1, public Interface2 {
 public:
  virtual void Do() const {
    cerr << "Does" << endl;
  }
};

int main() {
  Does* d = new Does;
  d->Do();

  Interface1* one = d;
  one->Do();

  Interface2* two = d;
  two->Do();

  return 0;
}

IIRC, we deliberately picked the same names in a few interfaces that
were commonly implemented together so we could exploit this. This was
what I was trying to do.

> http://codereview.chromium.org/2800015/show

[Evan Stade, 2010-06-24 19:34:26]

ok, thanks for answering my Qs. LGTM

http://codereview.chromium.org/2800015/diff/34002/35014
File chrome/browser/gtk/gtk_util.cc (right):

http://codereview.chromium.org/2800015/diff/34002/35014#newcode474
chrome/browser/gtk/gtk_util.cc:474: std::string
RemoveWindowsStyleAccelerators(const std::string& label) {
can we share the code with the above? make a
ReplaceWindowsStyleAcceleratorWith() helper perhaps?