[Mac] Replace MainMenu.xib with code to create the main menu bar.

This will eventually allow us to remove all XIB files after MacViewsBrowser
is fully launched.

This also now instantiates and leaks the AppController directly, rather
than it being a top-level object in the NIB.

Bug: 832676
Change-Id: I1c8ebdff45a92d4aee0f2a8b253274c03a76611c
Reviewed-on: https://chromium-review.googlesource.com/1147157
Commit-Queue: Robert Sesek <rsesek@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Reviewed-by: Erik Chen <erikchen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#577666}

chrome/app/chrome_command_ids.h

@@ -8,10 +8,6 @@
 // This file lists all the command IDs understood by e.g. the browser.
 // It is used by Windows RC files, Mac NIB files, and other platforms too.
 
-// Mac NIB files (e.g. chrome/app/nibs/MainMenu.xib) include ID numbers rather
-// than the corresponding #define labels.  If you change a given command's
-// number, any NIB files that refer to it will also need to be updated.
-
 // clang-format off
 
 // Values below IDC_MinimumLabelValue are reserved for dynamic menu items.

chrome/app/nibs/BUILD.gn

@@ -15,7 +15,6 @@ translated_xibs = [
   "DownloadItem.xib",
   "DownloadShelf.xib",
   "HungRendererDialog.xib",
-  "MainMenu.xib",
   "OneClickSigninBubble.xib",
   "OneClickSigninDialog.xib",
   "SaveAccessoryView.xib",

chrome/app/nibs/PRESUBMIT.py

@@ -26,10 +26,8 @@ IB_VERSION_RE = \
     'version="([0-9]+)"/>'
 
 def _CheckXIBSystemAndXcodeVersions(input_api, output_api, error_type):
-  # Skip MainMenu.xib, which was updated to use 10.3.3, Xcode 8.3.2.
   affected_xibs = [x for x in input_api.AffectedFiles()
-      if x.LocalPath().endswith('.xib')
-      and not x.LocalPath().endswith('MainMenu.xib')]
+      if x.LocalPath().endswith('.xib')]
 
   incorrect_system_versions = []
   incorrect_ib_versions = []

chrome/browser/app_controller_mac.h

@@ -83,10 +83,6 @@ class ScopedKeepAlive;
   IBOutlet NSMenuItem* closeTabMenuItem_;
   IBOutlet NSMenuItem* closeWindowMenuItem_;
 
-  // Outlet for the help menu so we can bless it so Cocoa adds the search item
-  // to it.
-  IBOutlet NSMenu* helpMenu_;
-
   // If we are expecting a workspace change in response to a reopen
   // event, the time we got the event. A null time otherwise.
   base::TimeTicks reopenTime_;
@@ -114,6 +110,10 @@ class ScopedKeepAlive;
 @property(readonly, nonatomic) BOOL startupComplete;
 @property(readonly, nonatomic) Profile* lastProfile;
 
+// This method is called very early in application startup after the main menu
+// has been created.
+- (void)mainMenuCreated;
+
 - (void)didEndMainMessageLoop;
 
 // Try to close all browser windows, and if that succeeds then quit.

chrome/browser/app_controller_mac.mm

@@ -329,7 +329,7 @@ static base::mac::ScopedObjCClassSwizzler* g_swizzle_imk_input_session;
 // This method is called very early in application startup (ie, before
 // the profile is loaded or any preferences have been registered). Defer any
 // user-data initialization until -applicationDidFinishLaunching:.
-- (void)awakeFromNib {
+- (void)mainMenuCreated {
   MacStartupProfiler::GetInstance()->Profile(
       MacStartupProfiler::AWAKE_FROM_NIB);
   // We need to register the handlers early to catch events fired on launch.
@@ -771,10 +771,6 @@ static base::mac::ScopedObjCClassSwizzler* g_swizzle_imk_input_session;
   profileAttributesStorageObserver_.reset(new AppControllerProfileObserver(
       g_browser_process->profile_manager(), self));
 
-  // Since Chrome is localized to more languages than the OS, tell Cocoa which
-  // menu is the Help so it can add the search item to it.
-  [NSApp setHelpMenu:helpMenu_];
-
   // Record the path to the (browser) app bundle; this is used by the app mode
   // shim.
   base::PostTaskWithTraits(FROM_HERE,

chrome/browser/app_controller_mac_browsertest.mm

@@ -684,7 +684,7 @@ IN_PROC_BROWSER_TEST_F(AppControllerMainMenuBrowserTest,
       [[NSApplication sharedApplication] delegate]);
   ASSERT_TRUE(ac);
 
-  [ac awakeFromNib];
+  [ac mainMenuCreated];
 
   // Constants for bookmarks that we will create later.
   const base::string16 title1(base::ASCIIToUTF16("Dinosaur Comics"));

chrome/browser/chrome_browser_main_mac.mm

@@ -27,6 +27,7 @@
 #include "chrome/browser/mac/keychain_reauthorize.h"
 #import "chrome/browser/mac/keystone_glue.h"
 #include "chrome/browser/mac/mac_startup_profiler.h"
+#include "chrome/browser/ui/cocoa/main_menu_builder.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/chrome_switches.h"
 #include "components/crash/content/app/crashpad.h"
@@ -129,18 +130,13 @@ void ChromeBrowserMainPartsMac::PreMainMessageLoopStart() {
     }
   }
 
-  // Now load the nib (from the right bundle).
-  base::scoped_nsobject<NSNib> nib(
-      [[NSNib alloc] initWithNibNamed:@"MainMenu"
-                               bundle:base::mac::FrameworkBundle()]);
-  // TODO(viettrungluu): crbug.com/20504 - This currently leaks, so if you
-  // change this, you'll probably need to change the Valgrind suppression.
-  NSArray* top_level_objects = nil;
-  [nib instantiateWithOwner:NSApp topLevelObjects:&top_level_objects];
-  for (NSObject* object : top_level_objects)
-    [object retain];
-  // Make sure the app controller has been created.
-  DCHECK([NSApp delegate]);
+  // Create the app delegate. This object is intentionally leaked as a global
+  // singleton. It is accessed through -[NSApp delegate].
+  AppController* app_controller = [[AppController alloc] init];
+  [NSApp setDelegate:app_controller];
+
+  chrome::BuildMainMenu(NSApp, app_controller);
+  [app_controller mainMenuCreated];
 
   // Do Keychain reauthorization. This gets two chances to run. If the first
   // try doesn't complete successfully (crashes or is interrupted for any

chrome/browser/ui/BUILD.gn

@@ -2463,6 +2463,8 @@ jumbo_split_static_library("ui") {
       "cocoa/l10n_util.mm",
       "cocoa/last_active_browser_cocoa.cc",
       "cocoa/last_active_browser_cocoa.h",
+      "cocoa/main_menu_builder.h",
+      "cocoa/main_menu_builder.mm",
       "cocoa/md_hover_button.h",
       "cocoa/md_hover_button.mm",
       "cocoa/md_util.h",

chrome/browser/ui/cocoa/accelerators_cocoa.mm

@@ -54,7 +54,8 @@ const struct AcceleratorMapping {
     {IDC_ZOOM_MINUS, ui::EF_COMMAND_DOWN, ui::VKEY_OEM_MINUS},
     {IDC_ZOOM_PLUS, ui::EF_COMMAND_DOWN | ui::EF_SHIFT_DOWN, ui::VKEY_OEM_PLUS},
 
-    // Accelerators used in MainMenu.xib, but not the toolbar menu.
+    // Accelerators used in the Main Menu, but not the toolbar menu.
+    {IDC_OPTIONS, ui::EF_COMMAND_DOWN, ui::VKEY_OEM_COMMA},
     {IDC_HIDE_APP, ui::EF_COMMAND_DOWN, ui::VKEY_H},
     {IDC_EXIT, ui::EF_COMMAND_DOWN, ui::VKEY_Q},
     {IDC_OPEN_FILE, ui::EF_COMMAND_DOWN, ui::VKEY_O},

chrome/browser/ui/cocoa/main_menu_builder.h

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROME_BROWSER_UI_COCOA_MAIN_MENU_BUILDER_H_
+#define CHROME_BROWSER_UI_COCOA_MAIN_MENU_BUILDER_H_
+
+#import <Cocoa/Cocoa.h>
+
+#include <vector>
+
+#include "base/mac/scoped_nsobject.h"
+#include "base/optional.h"
+
+@class AppController;
+
+namespace chrome {
+
+// Creates the main menu bar for and registers it as such on |nsapp|. The
+// NSApplicationDelegate |app_controller| is the target for specific,
+// special menu items.
+//
+// Normally the main menu is built in a MainMenu.nib file, but NIB files files
+// are hard to edit (especially cross-platform) and bring in a compile
+// dependency on ibtool. Building the menu in code has a lower maintenance
+// burden.
+void BuildMainMenu(NSApplication* nsapp, AppController* app_controller);
+
+// Internal ////////////////////////////////////////////////////////////////////
+
+namespace internal {
+
+// Helper class that builds NSMenuItems from data. Instances of this class
+// should not outlive an autorelease pool scope as it does not retain any
+// Objective-C members.
+//
+// This builder follows a fluent-interface pattern where the setters are
+// not prefixed with the typical "set_" and they return a reference to this
+// for easier method chaining.
+//
+// This class is only exposed for testing.
+class MenuItemBuilder {
+ public:
+  explicit MenuItemBuilder(int string_id = 0);
+
+  MenuItemBuilder(const MenuItemBuilder&);
+  MenuItemBuilder& operator=(const MenuItemBuilder&);
+
+  ~MenuItemBuilder();
+
+  // Converts the item to a separator. Only tag() is also applicable.
+  MenuItemBuilder& is_separator() {
+    DCHECK_EQ(string_id_, 0);
+    is_separator_ = true;
+    return *this;
+  }
+
+  MenuItemBuilder& target(id target) {
+    target_ = target;
+    return *this;
+  }
+
+  MenuItemBuilder& action(SEL action) {
+    DCHECK(!action_);
+    action_ = action;
+    return *this;
+  }
+
+  MenuItemBuilder& tag(int tag) {
+    tag_ = tag;
+    return *this;
+  }
+
+  // Wires up the menu item to the CommandDispatcher based on an
+  // IDC_ command code.
+  MenuItemBuilder& command_id(int command_id) {
+    return tag(command_id).action(@selector(commandDispatch:));
+  }
+
+  // Specifies a format string argument for the constructor's |string_id|.
+  MenuItemBuilder& string_format_1(int arg1) {
+    string_arg1_ = arg1;
+    return *this;
+  }
+
+  MenuItemBuilder& submenu(std::vector<MenuItemBuilder> items) {
+    submenu_ = std::move(items);
+    return *this;
+  }
+
+  // Registers a custom key equivalent. Normally the key equivalent is looked
+  // up via AcceleratorsCocoa based on the command_id(). If one is not present,
+  // the one specified here is used instead.
+  MenuItemBuilder& key_equivalent(NSString* key_equivalent,
+                                  NSEventModifierFlags flags) {
+    key_equivalent_ = key_equivalent;
+    key_equivalent_flags_ = flags;
+    return *this;
+  }
+
+  // Marks the item as an alternate keyboard equivalent menu item.
+  MenuItemBuilder& is_alternate() {
+    is_alternate_ = true;
+    return *this;
+  }
+
+  // Builds a NSMenuItem instance from the properties set on the Builder.
+  base::scoped_nsobject<NSMenuItem> Build() const;
+
+ private:
+  bool is_separator_ = false;
+
+  int string_id_ = 0;
+  int string_arg1_ = 0;
+
+  int tag_ = 0;
+
+  id target_ = nil;
+  SEL action_ = nil;
+
+  NSString* key_equivalent_ = @"";
+  NSEventModifierFlags key_equivalent_flags_ = 0;
+
+  bool is_alternate_ = false;
+
+  base::Optional<std::vector<MenuItemBuilder>> submenu_;
+
+  // Copy and assign allowed.
+};
+
+}  // namespace internal
+}  // namespace chrome
+
+#endif  // CHROME_BROWSER_UI_COCOA_MAIN_MENU_BUILDER_H_

chrome/browser/ui/cocoa/main_menu_builder_unittest.mm

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#import "chrome/browser/ui/cocoa/main_menu_builder.h"
+
+#include "chrome/app/chrome_command_ids.h"
+#include "chrome/grit/chromium_strings.h"
+#include "chrome/grit/generated_resources.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "testing/gtest_mac.h"
+#include "ui/base/l10n/l10n_util.h"
+#include "ui/base/l10n/l10n_util_mac.h"
+
+namespace {
+
+using chrome::internal::MenuItemBuilder;
+
+TEST(MainMenuBuilderTest, Separator) {
+  base::scoped_nsobject<NSMenuItem> item =
+      MenuItemBuilder().is_separator().Build();
+  EXPECT_TRUE([item isSeparatorItem]);
+  EXPECT_EQ(0, [item tag]);
+}
+
+TEST(MainMenuBuilderTest, SeparatorWithTag) {
+  base::scoped_nsobject<NSMenuItem> item =
+      MenuItemBuilder().is_separator().tag(999).Build();
+  EXPECT_TRUE([item isSeparatorItem]);
+  EXPECT_EQ(999, [item tag]);
+}
+
+TEST(MainMenuBuilderTest, CommandId) {
+  base::scoped_nsobject<NSMenuItem> item =
+      MenuItemBuilder(IDS_NEW_TAB).command_id(IDC_NEW_TAB).Build();
+  EXPECT_EQ(@selector(commandDispatch:), [item action]);
+  EXPECT_FALSE([item target]);
+  EXPECT_NSEQ(l10n_util::GetNSStringWithFixup(IDS_NEW_TAB), [item title]);
+  EXPECT_EQ(IDC_NEW_TAB, [item tag]);
+  EXPECT_NSEQ(@"t", [item keyEquivalent]);
+  EXPECT_EQ(NSEventModifierFlagCommand, [item keyEquivalentModifierMask]);
+}
+
+TEST(MainMenuBuilderTest, CustomTargetAction) {
+  base::scoped_nsobject<NSObject> target([[NSObject alloc] init]);
+
+  base::scoped_nsobject<NSMenuItem> item = MenuItemBuilder(IDS_PREFERENCES)
+                                               .target(target)
+                                               .action(@selector(fooBar:))
+                                               .Build();
+  EXPECT_NSEQ(l10n_util::GetNSStringWithFixup(IDS_PREFERENCES), [item title]);
+
+  EXPECT_EQ(target.get(), [item target]);
+  EXPECT_EQ(@selector(fooBar:), [item action]);
+  EXPECT_EQ(0, [item tag]);
+}
+
+TEST(MainMenuBuilderTest, Submenu) {
+  base::scoped_nsobject<NSMenuItem> item =
+      MenuItemBuilder(IDS_EDIT)
+          .tag(123)
+          .submenu({
+            MenuItemBuilder(IDS_CUT).tag(456).action(@selector(first:)),
+                MenuItemBuilder(IDS_COPY).tag(789).action(@selector(second:)),
+          })
+          .Build();
+
+  EXPECT_EQ(123, [item tag]);
+  EXPECT_NSEQ(l10n_util::GetNSStringWithFixup(IDS_EDIT), [item title]);
+  // These are hooked up by AppKit's -setSubmenu:.
+  EXPECT_EQ([item submenu], [item target]);
+  EXPECT_EQ(@selector(submenuAction:), [item action]);
+
+  NSMenu* submenu = [item submenu];
+  EXPECT_TRUE(submenu);
+
+  ASSERT_EQ(2u, [submenu numberOfItems]);
+
+  NSMenuItem* subitem = [submenu itemAtIndex:0];
+  EXPECT_EQ(456, [subitem tag]);
+  EXPECT_EQ(@selector(first:), [subitem action]);
+  EXPECT_NSEQ(l10n_util::GetNSStringWithFixup(IDS_CUT), [subitem title]);
+
+  subitem = [submenu itemAtIndex:1];
+  EXPECT_EQ(789, [subitem tag]);
+  EXPECT_EQ(@selector(second:), [subitem action]);
+  EXPECT_NSEQ(l10n_util::GetNSStringWithFixup(IDS_COPY), [subitem title]);
+}
+
+TEST(MainMenuBuilderTest, StringId) {
+  base::scoped_nsobject<NSMenuItem> item =
+      MenuItemBuilder(IDS_NEW_TAB_MAC).Build();
+  EXPECT_NSEQ(l10n_util::GetNSStringWithFixup(IDS_NEW_TAB_MAC), [item title]);
+}
+
+TEST(MainMenuBuilderTest, StringIdWithArg) {
+  base::scoped_nsobject<NSMenuItem> item =
+      MenuItemBuilder(IDS_ABOUT_MAC).string_format_1(IDS_PRODUCT_NAME).Build();
+
+  base::string16 product_name = l10n_util::GetStringUTF16(IDS_PRODUCT_NAME);
+  EXPECT_NSEQ(l10n_util::GetNSStringF(IDS_ABOUT_MAC, product_name),
+              [item title]);
+}
+
+}  // namespace

chrome/test/BUILD.gn

@@ -4152,6 +4152,7 @@ test("unit_tests") {
         "../browser/ui/cocoa/location_bar/page_info_bubble_decoration_unittest.mm",
         "../browser/ui/cocoa/location_bar/selected_keyword_decoration_unittest.mm",
         "../browser/ui/cocoa/location_bar/zoom_decoration_unittest.mm",
+        "../browser/ui/cocoa/main_menu_builder_unittest.mm",
         "../browser/ui/cocoa/md_hover_button_unittest.mm",
         "../browser/ui/cocoa/media_picker/desktop_media_picker_controller_unittest.mm",
         "../browser/ui/cocoa/menu_button_unittest.mm",