Port the Menu class to GTK.

Create a menu wrapper class for GTK menus, and use it for the page and app menus.

[Jo, 2009-02-11 03:53:32]

Your hack to build menu.cc in browser.scons can be fixed by removing the line
214 in chrome/views/SConscript.

http://codereview.chromium.org/20245/diff/26/1001
File chrome/browser/browser.scons (right):

http://codereview.chromium.org/20245/diff/26/1001#newcode806
Line 806: # build it. For now, do this ugly hack.
You could remove the file from chrome/views/SConscript @ line 213.

http://codereview.chromium.org/20245/diff/26/1008
File chrome/views/SConscript (right):

http://codereview.chromium.org/20245/diff/26/1008#newcode214
Line 214: 'menu.cc',
This is where you will "force" it to build by removing this line.

[Elliot Glaysher, 2009-02-11 07:23:20]

http://codereview.chromium.org/20245/diff/26/1001
File chrome/browser/browser.scons (right):

http://codereview.chromium.org/20245/diff/26/1001#newcode806
Line 806: # build it. For now, do this ugly hack.
On 2009/02/11 03:53:33, Jo wrote:
> You could remove the file from chrome/views/SConscript @ line 213.

I'm pretty sure I can't do that without breaking the scons build on Windows.
Hence, the hack right here.

Also, this patch isn't really out for review...yet. I uploaded it to show
another chromium dev since I wasn't sure what to do with the gtk_menu_popup()
callback.

[Elliot Glaysher, 2009-02-12 02:04:32]

I have just realized what you meant and that you are correct. Not being tired
can make a big difference...

On 2009/02/11 03:53:32, Jo wrote:
> Your hack to build menu.cc in browser.scons can be fixed by removing the line
> 214 in chrome/views/SConscript.
> 
> http://codereview.chromium.org/20245/diff/26/1001
> File chrome/browser/browser.scons (right):
> 
> http://codereview.chromium.org/20245/diff/26/1001#newcode806
> Line 806: # build it. For now, do this ugly hack.
> You could remove the file from chrome/views/SConscript @ line 213.
> 
> http://codereview.chromium.org/20245/diff/26/1008
> File chrome/views/SConscript (right):
> 
> http://codereview.chromium.org/20245/diff/26/1008#newcode214
> Line 214: 'menu.cc',
> This is where you will "force" it to build by removing this line.

[Evan Stade, 2009-02-12 05:00:24]

So are we porting views then? I don't quite see the point of porting the views
API if we are going to segregate the calling code by platform anyway---or is
there some shared code that calls uses the Menu class?

Side note: the directory structure seems a little
strange---chrome/view/menu_win.cc, but chrome/browser/gtk/menu_gtk.cc? Is that
just a temporary thing?

(also, lint errors)

http://codereview.chromium.org/20245/diff/2001/3003
File chrome/browser/gtk/browser_toolbar_view_gtk.cc (right):

http://codereview.chromium.org/20245/diff/2001/3003#newcode13
Line 13: #include "chrome/views/menu.h"
I thought we were not porting views (or hadn't decided on it). Clearly this
changelist hinges on it though---are we going to half-port views then?

http://codereview.chromium.org/20245/diff/2001/3003#newcode184
Line 184: l10n_util::GetString(IDS_PAGEMENU_TOOLTIP),
the previous tabbing was correct (here and below)

http://codereview.chromium.org/20245/diff/2001/3003#newcode282
Line 282: if (tag != -1) {
I'd say we should DCHECK this if condition

http://codereview.chromium.org/20245/diff/2001/3003#newcode308
Line 308: void BrowserToolbarGtk::RunPageMenu(GdkEvent* button_press_event) {
You might pull out each submenu init block into a static helper function to
improve readability.

Or better yet, you could make an array of MenuCreateMaterial structs and just
iterate over them.

http://codereview.chromium.org/20245/diff/2001/3003#newcode357
Line 357: for (int i = 0; i < 4; ++i) {
use arraysize(developer_menu_materials) rather than 4

http://codereview.chromium.org/20245/diff/2001/3004
File chrome/browser/gtk/browser_toolbar_view_gtk.h (right):

http://codereview.chromium.org/20245/diff/2001/3004#newcode52
Line 52: static gint MenuButtonPressCallback(GtkWidget* button,
I think our naming scheme for callbacks would dictate these being called
ButtonClicked and MenuButtonPressed

[Elliot Glaysher, 2009-02-12 21:50:32]

I've just uploaded the first version I'd like to get reviews on. (I'll make sure
to leave a message in the description next time I upload unfinished work).

This is not porting the views hierarchy. Menu is not a View; it's a stand alone
class that doesn't have any other dependencies. I ported this since I needed a
simplified wrapper class for generating and managing GtkMenuItems anyway and
since I was hoping to reuse the same code that generated the menus. (Now
factored into views/standard_menus.h).

[Evan Martin, 2009-02-12 22:13:35]

http://codereview.chromium.org/20245/diff/1052/1056
File chrome/browser/gtk/browser_toolbar_view_gtk.cc (right):

http://codereview.chromium.org/20245/diff/1052/1056#newcode26
Line 26: explicit CustomDrawButton(const std::string& filename);
nice catch!

http://codereview.chromium.org/20245/diff/1052/1056#newcode247
Line 247: bool menu_button) {
i wonder if it'd be better to just connect the signals above and remove this
function.  it doesn't do *that* much, and we're going to need even more custom
logic to make the menu buttons render right.

http://codereview.chromium.org/20245/diff/1052/1056#newcode287
Line 287: GdkEvent *event,
star on left (here and below)

http://codereview.chromium.org/20245/diff/1052/1057
File chrome/browser/gtk/browser_toolbar_view_gtk.h (right):

http://codereview.chromium.org/20245/diff/1052/1057#newcode9
Line 9: #include <string>
i think we do c++ headers, newline, c headers.  i forget though so ignore me if
i'm wrong.

http://codereview.chromium.org/20245/diff/1052/1057#newcode80
Line 80: scoped_ptr<Menu> page_menu_;
comments?  (not that it isn't obvious, but maybe you could note whether they're
ever expected to be null, etc.)

http://codereview.chromium.org/20245/diff/1052/1062
File chrome/views/SConscript (right):

http://codereview.chromium.org/20245/diff/1052/1062#newcode137
Line 137: 'standard_menus.h',
These are browser-specific menu constructors, right?  I believe they belong in
chrome/browser/views instead of the higher-level chrome/views.  The former is
for browser-specific constructions, the latter for platform abstractions.

http://codereview.chromium.org/20245/diff/1052/1064
File chrome/views/menu.h (right):

http://codereview.chromium.org/20245/diff/1052/1064#newcode26
Line 26: // A Note: You can't actually *do* this in standard C++!
Really?  I think you can use anon namespaces to forward-declare within a file...
?

http://codereview.chromium.org/20245/diff/1052/1064#newcode44
Line 44: //   A wrapper around a Win32 HMENU handle that provides convenient
APIs for
this comment needs updating

http://codereview.chromium.org/20245/diff/1052/1064#newcode367
Line 367: static void OnMenuItemActivated(GtkMenuItem *menuitem, ItemData*
data);
stars on left here and below

http://codereview.chromium.org/20245/diff/1052/1064#newcode383
Line 383: std::map<GtkWidget*, ItemData*> item_data_;
you can use
http://library.gnome.org/devel/gobject/stable/gobject-The-Base-Object-Type.ht...
to associate a pointer with an object.

http://codereview.chromium.org/20245/diff/1052/1065
File chrome/views/menu_gtk.cc (right):

http://codereview.chromium.org/20245/diff/1052/1065#newcode87
Line 87: menu_item = gtk_menu_item_new_with_label(utf8_label->c_str());
gtk always makes copies of passed-in pointers, so this converted_labels stuff
isn't necessary.

(i think what you have also wouldn't necessarily work as you expect -- when more
entries are added to converted_labels_, it has to resize the vector, sometimes
copying its strings around so that their c_str() pointers would change.)

http://codereview.chromium.org/20245/diff/1052/1065#newcode151
Line 151: GdkEventButton *event_button =
reinterpret_cast<GdkEventButton*>(event);
star on left

http://codereview.chromium.org/20245/diff/1052/1068
File chrome/views/standard_menus.h (right):

http://codereview.chromium.org/20245/diff/1052/1068#newcode13
Line 13: Menu* BuildStandardAppMenuReturningProfilesMenu(Menu* menu);
This is a bit weird.  I'd say just make it take a Menu** to return profiles via.

[Evan Stade, 2009-02-12 22:23:12]

http://codereview.chromium.org/20245/diff/1052/1064
File chrome/views/menu.h (right):

http://codereview.chromium.org/20245/diff/1052/1064#newcode26
Line 26: // A Note: You can't actually *do* this in standard C++!
On 2009/02/12 22:13:35, Evan Martin wrote:
> Really?  I think you can use anon namespaces to forward-declare within a
file...
> ?

I think the problem is that MenuHostWindow is actually declared in an anonymous
namespace in menu.cc. Given that this is a header file, there'd be no point in
wrapping this forward declaration in an anon namespace otherwise (right?).

http://codereview.chromium.org/20245/diff/1052/1067
File chrome/views/standard_menus.cc (right):

http://codereview.chromium.org/20245/diff/1052/1067#newcode81
Line 81: Menu* zoom_menu = menu->AppendSubMenu(IDC_ZOOM_MENU,
I think I'd prefer if these submenus were also packed into a struct. Then you
could see the entire StandardPageMenu hierarchy just by reading the struct.

http://codereview.chromium.org/20245/diff/1052/1067#newcode115
Line 115: menu->AppendMenuItemWithLabel(IDC_CLEAR_BROWSING_DATA,
why aren't the rest of these in their own struct?

[Evan Martin, 2009-02-12 23:04:36]

As I was saying in chat, I think what we want to do is:

- share standard_menus.cc's data structures of what menu items go where
- use the same delegates where appropriate
- have a separate implementation of the thing that creates a menu, and create a
new menu each time someone clicks on the button.  this lets us just set the
state of each widget as we're bringing up the menu, and means we don't need to
track any state.

The GTK model is reference-counted such that submenus are handled implicitly by
their parent and gtk_menu_popup will destroy it.  (If I'm wrong on the latter
bit, then you can just make the "destroy" signal on the GtkMenu* destroy it.) 
Typically in GTK apps you don't ever need to keep a pointer around (unless you
want to reference the object later) and it'll destroy itself.  I think that
means the toolbar doesn't need a Menu* at all.

[Elliot Glaysher, 2009-02-13 22:45:32]

I've redone how the Menus work so the MenuGtk class isn't an implementation of
Menu with all the windowisms that entails.
(We need to manage the lifetime of the GtkMenu* ourselves because the menubar
usually owns the Menu. I talked to evanm about this yesterday.) 

The standard menus file may need to be moved to chrome/common/. Currently it's
only used by GTK though.

http://codereview.chromium.org/20245/diff/1052/1062
File chrome/views/SConscript (right):

http://codereview.chromium.org/20245/diff/1052/1062#newcode137
Line 137: 'standard_menus.h',
On 2009/02/12 22:13:35, Evan Martin wrote:
> These are browser-specific menu constructors, right?  I believe they belong in
> chrome/browser/views instead of the higher-level chrome/views.  The former is
> for browser-specific constructions, the latter for platform abstractions.

Done.

http://codereview.chromium.org/20245/diff/1052/1064
File chrome/views/menu.h (right):

http://codereview.chromium.org/20245/diff/1052/1064#newcode26
Line 26: // A Note: You can't actually *do* this in standard C++!
On 2009/02/12 22:23:12, estade wrote:
> On 2009/02/12 22:13:35, Evan Martin wrote:
> > Really?  I think you can use anon namespaces to forward-declare within a
> file...
> > ?
> 
> I think the problem is that MenuHostWindow is actually declared in an
anonymous
> namespace in menu.cc. Given that this is a header file, there'd be no point in
> wrapping this forward declaration in an anon namespace otherwise (right?).

You're not supposed to forward declare in header files since it leads to weird
behaviour. Estade is correct.

http://codereview.chromium.org/20245/diff/1052/1064#newcode44
Line 44: //   A wrapper around a Win32 HMENU handle that provides convenient
APIs for
On 2009/02/12 22:13:35, Evan Martin wrote:
> this comment needs updating

Done.

http://codereview.chromium.org/20245/diff/1052/1064#newcode367
Line 367: static void OnMenuItemActivated(GtkMenuItem *menuitem, ItemData*
data);
On 2009/02/12 22:13:35, Evan Martin wrote:
> stars on left here and below

Note to self: can't just copy the function prototypes from the GTK docs...

[Evan Stade, 2009-02-13 23:05:21]

I like!

LG with the following nits

http://codereview.chromium.org/20245/diff/1052/1064
File chrome/views/menu.h (right):

http://codereview.chromium.org/20245/diff/1052/1064#newcode26
Line 26: // A Note: You can't actually *do* this in standard C++!
> You're not supposed to forward declare in header files since it leads to weird
> behaviour. Estade is correct.

we forward declare in header files all the time... I thought the problem here
was just this anonymous namespace/friend class trickery

http://codereview.chromium.org/20245/diff/72/1092
File chrome/browser/gtk/browser_toolbar_view_gtk.cc (right):

http://codereview.chromium.org/20245/diff/72/1092#newcode289
Line 289: DCHECK(tag != -1) << "Impossible button click callback.";
shouldn't this be DCHECK(tag == -1)

http://codereview.chromium.org/20245/diff/72/1094
File chrome/browser/gtk/menu_gtk.cc (right):

http://codereview.chromium.org/20245/diff/72/1094#newcode24
Line 24: << "Non-button press event sent to RunMenuAt!";
nit: are these ! marks at the end of error messages standard? They seem
extravagant

http://codereview.chromium.org/20245/diff/72/1094#newcode48
Line 48: DCHECK(menu_data->type == MENU_SEPARATOR) << "Menu definition borken!";
error message borken :)

[Elliot Glaysher, 2009-02-13 23:21:53]

http://codereview.chromium.org/20245/diff/1052/1064
File chrome/views/menu.h (right):

http://codereview.chromium.org/20245/diff/1052/1064#newcode26
Line 26: // A Note: You can't actually *do* this in standard C++!
On 2009/02/13 23:05:22, estade wrote:
> > You're not supposed to forward declare in header files since it leads to
weird
> > behaviour. Estade is correct.
> 
> we forward declare in header files all the time... I thought the problem here
> was just this anonymous namespace/friend class trickery
> 

I meant forward declaring in an annonymous namespace. There's no problem forward
declaring in the global or a named namespace.

http://codereview.chromium.org/20245/diff/72/1092
File chrome/browser/gtk/browser_toolbar_view_gtk.cc (right):

http://codereview.chromium.org/20245/diff/72/1092#newcode289
Line 289: DCHECK(tag != -1) << "Impossible button click callback.";
On 2009/02/13 23:05:22, estade wrote:
> shouldn't this be DCHECK(tag == -1)

I thought DCHECKs triggered on DCHECK(false)... The macro in logging.h says that
a CHECK expands to LOG_IF(FATAL, !(condition))...

http://codereview.chromium.org/20245/diff/72/1094
File chrome/browser/gtk/menu_gtk.cc (right):

http://codereview.chromium.org/20245/diff/72/1094#newcode24
Line 24: << "Non-button press event sent to RunMenuAt!";
On 2009/02/13 23:05:22, estade wrote:
> nit: are these ! marks at the end of error messages standard? They seem
> extravagant

Done.

http://codereview.chromium.org/20245/diff/72/1094#newcode48
Line 48: DCHECK(menu_data->type == MENU_SEPARATOR) << "Menu definition borken!";
On 2009/02/13 23:05:22, estade wrote:
> error message borken :)

It was deliberate, but I've pulled it out.

[Evan Stade, 2009-02-13 23:28:46]

http://codereview.chromium.org/20245/diff/72/1092
File chrome/browser/gtk/browser_toolbar_view_gtk.cc (right):

http://codereview.chromium.org/20245/diff/72/1092#newcode289
Line 289: DCHECK(tag != -1) << "Impossible button click callback.";
On 2009/02/13 23:21:53, Elliot Glaysher wrote:
> On 2009/02/13 23:05:22, estade wrote:
> > shouldn't this be DCHECK(tag == -1)
> 
> I thought DCHECKs triggered on DCHECK(false)... The macro in logging.h says
that
> a CHECK expands to LOG_IF(FATAL, !(condition))...

o rite, /ignore me

[Evan Martin, 2009-02-13 23:37:21]

looks good to me

http://codereview.chromium.org/20245/diff/1099/1101
File chrome/browser/gtk/browser_toolbar_view_gtk.cc (right):

http://codereview.chromium.org/20245/diff/1099/1101#newcode119
Line 119: back_(NULL),
FWIW, the default constructor does the right thing for these.

http://codereview.chromium.org/20245/diff/1099/1103
File chrome/browser/gtk/menu_gtk.cc (right):

http://codereview.chromium.org/20245/diff/1099/1103#newcode121
Line 121: // Regretfully, we can't rely on this. It'd make the above
calculations just
what does "this" refer to here?

http://codereview.chromium.org/20245/diff/1099/1105
File chrome/browser/gtk/standard_menus.cc (right):

http://codereview.chromium.org/20245/diff/1099/1105#newcode16
Line 16: struct MenuCreateMaterial zoom_menu_materials[] = {
is the intent to share this with windows?  otherwise there's that MenuFactory
thingy in GTK which may do what you need. (this is fine either way, just FYI)

http://codereview.chromium.org/20245/diff/1099/1105#newcode25
Line 25: { MENU_CHECKBOX, IDC_ENCODING_AUTO_DETECT, IDS_ENCODING_AUTO_DETECT, 0,
NULL },
80 cols

http://codereview.chromium.org/20245/diff/1099/1106
File chrome/browser/gtk/standard_menus.h (right):

http://codereview.chromium.org/20245/diff/1099/1106#newcode28
Line 28: unsigned int menu_label_id;
do these need "menu_" prefixes?

[Elliot Glaysher, 2009-02-17 17:59:27]

http://codereview.chromium.org/20245/diff/1099/1101
File chrome/browser/gtk/browser_toolbar_view_gtk.cc (right):

http://codereview.chromium.org/20245/diff/1099/1101#newcode119
Line 119: back_(NULL),
On 2009/02/13 23:37:21, Evan Martin wrote:
> FWIW, the default constructor does the right thing for these.

These used to be raw pointers. Removed.

http://codereview.chromium.org/20245/diff/1099/1103
File chrome/browser/gtk/menu_gtk.cc (right):

http://codereview.chromium.org/20245/diff/1099/1103#newcode121
Line 121: // Regretfully, we can't rely on this. It'd make the above
calculations just
On 2009/02/13 23:37:21, Evan Martin wrote:
> what does "this" refer to here?

Done.

http://codereview.chromium.org/20245/diff/1099/1105
File chrome/browser/gtk/standard_menus.cc (right):

http://codereview.chromium.org/20245/diff/1099/1105#newcode16
Line 16: struct MenuCreateMaterial zoom_menu_materials[] = {
On 2009/02/13 23:37:21, Evan Martin wrote:
> is the intent to share this with windows?  otherwise there's that MenuFactory
> thingy in GTK which may do what you need. (this is fine either way, just FYI)

I intend to share this with windows.

[Evan Martin, 2009-02-17 19:08:24]

LGTM, minor nits follow

http://codereview.chromium.org/20245/diff/1110/1113
File chrome/browser/gtk/browser_toolbar_view_gtk.h (right):

http://codereview.chromium.org/20245/diff/1110/1113#newcode55
Line 55: // Gtk callback to intercept mouse clicks to the menu buttons.
i guess "intercept" is sort of the wrong word -- it's just responding to a
different event than normal.

http://codereview.chromium.org/20245/diff/1110/1113#newcode57
Line 57: GdkEvent *event,
tabbing weird here

[Dean McNamee, 2009-02-18 12:35:17]

http://codereview.chromium.org/20245/diff/1110/1114
File chrome/browser/gtk/menu_gtk.cc (right):

http://codereview.chromium.org/20245/diff/1110/1114#newcode94
Line 94: /* static */
I've seen this in a few places now.  I don't think we ever use C style /* */
comments.  This should probably be a // static comment to match the rest of our
code.