Make a shared app menu model and update win and mac to use it. Remove the NSM...

Make a shared app menu model and update win and mac to use it. Remove the NSMenu from Toolbar.xib. 
BUG=22646
TEST=the app menu works as it used to.

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

[viettrungluu, 2009-12-10 05:50:23]

LGTM with stuff addressed and another pass through the trybots (I don't really
get why the patch failed to apply on some platforms but not others.)

http://codereview.chromium.org/482006/diff/6004/6011
File chrome/browser/app_menu_model.cc (right):

http://codereview.chromium.org/482006/diff/6004/6011#newcode43
chrome/browser/app_menu_model.cc:43: AddSubMenuWithStringId(IDS_PROFILE_MENU,
profiles_menu_contents_.get());
Is this AddSubMenuWithStringId() really meant do go under the condition
!profiles_menu_contents_.get()? It seems to me that this should be:

if (command_line.HasSwitch(...)) {
  if (!profiles_menu_contents_.get()) {
    profiles_helper_->...;
    profiles_menu_contents_.reset(...);
  }
  AddSubMenuWithStringId(...);
}

Am I missing something?

http://codereview.chromium.org/482006/diff/6004/6011#newcode66
chrome/browser/app_menu_model.cc:66: label = type == sync_ui_util::SYNCED ?
Do we really allow this sort of thing in our style guide? I would strongly
prefer if (...) ... else if (...) ... else ....

http://codereview.chromium.org/482006/diff/6004/6011#newcode101
chrome/browser/app_menu_model.cc:101: // Add direct sub menu items for profiles.
"Submenu" is usually one word.

http://codereview.chromium.org/482006/diff/6004/6011#newcode104
chrome/browser/app_menu_model.cc:104: (i <= IDC_NEW_WINDOW_PROFILE_LAST) &&
(iter != profiles.end());
The extra parens are redundant, even for me.

http://codereview.chromium.org/482006/diff/6004/6015
File chrome/browser/app_menu_model_unittest.cc (right):

http://codereview.chromium.org/482006/diff/6004/6015#newcode51
chrome/browser/app_menu_model_unittest.cc:51: delegate.enable_count_ = 0;
Is resetting these really necessary?

http://codereview.chromium.org/482006/diff/6004/6013
File chrome/browser/cocoa/toolbar_controller.h (right):

http://codereview.chromium.org/482006/diff/6004/6013#newcode60
chrome/browser/cocoa/toolbar_controller.h:60: // page and wrench buttons. If
it's visible, these will be non-null, but they
Could you mention that the wrench menu is also known as the app menu? (Hmmm. I
just noticed that it's presumably also called the tools menu, apparently. Or at
least the button is called the tools button.)

http://codereview.chromium.org/482006/diff/6004/6006
File chrome/browser/defaults.cc (right):

http://codereview.chromium.org/482006/diff/6004/6006#newcode22
chrome/browser/defaults.cc:22: const bool kOSSupportsOtherBrowsers = false;
Could you make the order of these three items (exit/about/other browsers) match
the order below?

http://codereview.chromium.org/482006/diff/6004/6008
File chrome/browser/defaults.h (right):

http://codereview.chromium.org/482006/diff/6004/6008#newcode44
chrome/browser/defaults.h:44: // bookmarks.
Drive-by fix?: ? not .

http://codereview.chromium.org/482006/diff/6004/6008#newcode47
chrome/browser/defaults.h:47: // Should the exit menu item be shown?
A little late ... but should it be shown in which menu?

http://codereview.chromium.org/482006/diff/6004/6008#newcode51
chrome/browser/defaults.h:51: extern const bool kShowAboutMenuItem;
Ditto.

http://codereview.chromium.org/482006/diff/6004/6010
File chrome/browser/views/toolbar_view.cc (left):

http://codereview.chromium.org/482006/diff/6004/6010#oldcode794
chrome/browser/views/toolbar_view.cc:794:
app_menu_contents_->AddSubMenuWithStringId(IDS_PROFILE_MENU,
Hmmm. It's like that here too. I wonder what I'm not getting.

http://codereview.chromium.org/482006/diff/6004/6010
File chrome/browser/views/toolbar_view.cc (right):

http://codereview.chromium.org/482006/diff/6004/6010#newcode113
chrome/browser/views/toolbar_view.cc:113: ToolbarView::~ToolbarView() {
You should probably just put this in the header file now.

[pink (ping after 24hrs), 2009-12-10 21:20:27]

New version pushed up with everything resolved. Take another look.

http://codereview.chromium.org/482006/diff/6004/6011
File chrome/browser/app_menu_model.cc (right):

http://codereview.chromium.org/482006/diff/6004/6011#newcode43
chrome/browser/app_menu_model.cc:43: AddSubMenuWithStringId(IDS_PROFILE_MENU,
profiles_menu_contents_.get());
yeah, this doesn't make any sense, i'll change it to what you suggest. Note I
just copied the (mostly deprecated) code.

http://codereview.chromium.org/482006/diff/6004/6011#newcode66
chrome/browser/app_menu_model.cc:66: label = type == sync_ui_util::SYNCED ?
On 2009/12/10 05:50:23, viettrungluu wrote:
> Do we really allow this sort of thing in our style guide? I would strongly
> prefer if (...) ... else if (...) ... else ....

Done.

http://codereview.chromium.org/482006/diff/6004/6011#newcode101
chrome/browser/app_menu_model.cc:101: // Add direct sub menu items for profiles.
On 2009/12/10 05:50:23, viettrungluu wrote:
> "Submenu" is usually one word.

Done.

http://codereview.chromium.org/482006/diff/6004/6011#newcode104
chrome/browser/app_menu_model.cc:104: (i <= IDC_NEW_WINDOW_PROFILE_LAST) &&
(iter != profiles.end());
On 2009/12/10 05:50:23, viettrungluu wrote:
> The extra parens are redundant, even for me.

Done.

http://codereview.chromium.org/482006/diff/6004/6015
File chrome/browser/app_menu_model_unittest.cc (right):

http://codereview.chromium.org/482006/diff/6004/6015#newcode51
chrome/browser/app_menu_model_unittest.cc:51: delegate.enable_count_ = 0;
On 2009/12/10 05:50:23, viettrungluu wrote:
> Is resetting these really necessary?

no, but in case someone wanted to add more cases to the end, then i didn't want
them to forget to reset the counters.

http://codereview.chromium.org/482006/diff/6004/6013
File chrome/browser/cocoa/toolbar_controller.h (right):

http://codereview.chromium.org/482006/diff/6004/6013#newcode60
chrome/browser/cocoa/toolbar_controller.h:60: // page and wrench buttons. If
it's visible, these will be non-null, but they
On 2009/12/10 05:50:23, viettrungluu wrote:
> Could you mention that the wrench menu is also known as the app menu? (Hmmm. I
> just noticed that it's presumably also called the tools menu, apparently. Or
at
> least the button is called the tools button.)

Done.

http://codereview.chromium.org/482006/diff/6004/6006
File chrome/browser/defaults.cc (right):

http://codereview.chromium.org/482006/diff/6004/6006#newcode22
chrome/browser/defaults.cc:22: const bool kOSSupportsOtherBrowsers = false;
On 2009/12/10 05:50:23, viettrungluu wrote:
> Could you make the order of these three items (exit/about/other browsers)
match
> the order below?

Done.

http://codereview.chromium.org/482006/diff/6004/6008
File chrome/browser/defaults.h (right):

http://codereview.chromium.org/482006/diff/6004/6008#newcode44
chrome/browser/defaults.h:44: // bookmarks.
On 2009/12/10 05:50:23, viettrungluu wrote:
> Drive-by fix?: ? not .

Done.

http://codereview.chromium.org/482006/diff/6004/6008#newcode47
chrome/browser/defaults.h:47: // Should the exit menu item be shown?
On 2009/12/10 05:50:23, viettrungluu wrote:
> A little late ... but should it be shown in which menu?

Done.

http://codereview.chromium.org/482006/diff/6004/6008#newcode51
chrome/browser/defaults.h:51: extern const bool kShowAboutMenuItem;
On 2009/12/10 05:50:23, viettrungluu wrote:
> Ditto.

Done.

http://codereview.chromium.org/482006/diff/6004/6010
File chrome/browser/views/toolbar_view.cc (right):

http://codereview.chromium.org/482006/diff/6004/6010#newcode113
chrome/browser/views/toolbar_view.cc:113: ToolbarView::~ToolbarView() {
On 2009/12/10 05:50:23, viettrungluu wrote:
> You should probably just put this in the header file now.

Done.

[viettrungluu, 2009-12-10 21:26:16]

LGTM. I'm happy if the trybots are happy. Actually, I'm probably happy even if
the trybots aren't happy (they're just so fickle).