Cache the bookmarks menu on OSX between profile switches.

Previously, when you switched to a profile, we invalidated the existing
bookmarks menu and updated it lazily. Because of the way OSX works, this causes
the menu to be recalculated from scratch the first time you press *any* keyboard
shortcut afer switching to the profile. On a profile with several hundred
bookmarks, this can take several seconds - more than enough to impact casual
browsing.
In particular, it would appear to the user that various innocent shortcuts (e.g.
opening a new tab, copying text, highlighting the location bar) are extremely
slow some of the time. Even more annoyingly, if you switched to a profile and
then immediately alt-tabbed away from it (for example, to get to a third
profile), the lockup would take place when you switched *away* from the profile
– even if you didn't perform any actions on the profile!

This change caches the bookmarks menu and restores it when a window for the
profile gains focus again. That way, the bookmarks menu is updated only when an
action directly invalidates it (i.e. adding or changing bookmarks themselves).
The accompanying test (BookmarksMenuIsRestoredAfterProfileSwitch) verifies that
the menu is restored correctly after switching profile windows and back.

Thanks to Robert Sesek for extensive help while writing and debugging this fix,
as well as its accompanying test. :-)

BUG=429371
TEST=Follow the steps in comment #27 on bug 429371: https://crbug.com/429371#c27

Review URL: https://codereview.chromium.org/733673003

Cr-Commit-Position: refs/heads/master@{#304531}

8 files changed, 118 insertions, 19 deletions

diff --git a/chrome/browser/OWNERS b/chrome/browser/OWNERS
index 9873dd6..2aafe2f 100644
--- a/chrome/browser/OWNERS
+++ b/chrome/browser/OWNERS
@@ -27,6 +27,11 @@ per-file *.mm=mark@chromium.org
 per-file *.mm=rsesek@chromium.org
 per-file *.mm=thakis@chromium.org
 
+per-file *_mac.h=avi@chromium.org
+per-file *_mac.h=mark@chromium.org
+per-file *_mac.h=rsesek@chromium.org
+per-file *_mac.h=thakis@chromium.org
+
 per-file shell_integration_win*=gab@chromium.org
 per-file shell_integration_win*=grt@chromium.org
 per-file shell_integration_linux*=erg@chromium.org
diff --git a/chrome/browser/app_controller_mac.h b/chrome/browser/app_controller_mac.h
index 8e4eccf..6eb522c 100644
--- a/chrome/browser/app_controller_mac.h
+++ b/chrome/browser/app_controller_mac.h
@@ -49,8 +49,13 @@ class WorkAreaWatcherObserver;
   scoped_ptr<AppControllerProfileObserver> profileInfoCacheObserver_;
 
   // Management of the bookmark menu which spans across all windows
-  // (and Browser*s).
-  scoped_ptr<BookmarkMenuBridge> bookmarkMenuBridge_;
+  // (and Browser*s). |profileBookmarkMenuBridgeMap_| is a cache that owns one
+  // pointer to a BookmarkMenuBridge for each profile. |bookmarkMenuBridge_| is
+  // a weak pointer that is updated to match the corresponding cache entry
+  // during a profile switch.
+  BookmarkMenuBridge* bookmarkMenuBridge_;
+  std::map<Profile*, BookmarkMenuBridge*> profileBookmarkMenuBridgeMap_;
+
   scoped_ptr<HistoryMenuBridge> historyMenuBridge_;
 
   // Controller that manages main menu items for packaged apps.
diff --git a/chrome/browser/app_controller_mac.mm b/chrome/browser/app_controller_mac.mm
index 2c17fa2..f234859 100644
--- a/chrome/browser/app_controller_mac.mm
+++ b/chrome/browser/app_controller_mac.mm
@@ -14,6 +14,7 @@
 #include "base/message_loop/message_loop.h"
 #include "base/metrics/histogram.h"
 #include "base/prefs/pref_service.h"
+#include "base/stl_util.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/sys_string_conversions.h"
 #include "base/strings/utf_string_conversions.h"
@@ -480,6 +481,9 @@ class AppControllerProfileObserver : public ProfileInfoCacheObserver {
   [self unregisterEventHandlers];
 
   appShimMenuController_.reset();
+
+  STLDeleteContainerPairSecondPointers(profileBookmarkMenuBridgeMap_.begin(),
+                                       profileBookmarkMenuBridgeMap_.end());
 }
 
 - (void)didEndMainMessageLoop {
@@ -887,6 +891,14 @@ class AppControllerProfileObserver : public ProfileInfoCacheObserver {
   // to the old profile.
   if (profilePath == lastProfile->GetPath())
     lastProfile_ = g_browser_process->profile_manager()->GetLastUsedProfile();
+
+  Profile* profile =
+      g_browser_process->profile_manager()->GetProfile(profilePath);
+  auto it = profileBookmarkMenuBridgeMap_.find(profile);
+  if (it != profileBookmarkMenuBridgeMap_.end()) {
+    delete it->second;
+    profileBookmarkMenuBridgeMap_.erase(it);
+  }
 }
 
 // Returns true if there is a modal window (either window- or application-
@@ -1520,7 +1532,7 @@ class AppControllerProfileObserver : public ProfileInfoCacheObserver {
 }
 
 - (BookmarkMenuBridge*)bookmarkMenuBridge {
-  return bookmarkMenuBridge_.get();
+  return bookmarkMenuBridge_;
 }
 
 - (void)addObserverForWorkAreaChange:(ui::WorkAreaWatcherObserver*)observer {
@@ -1540,18 +1552,26 @@ class AppControllerProfileObserver : public ProfileInfoCacheObserver {
   if (lastProfile_ == profile)
     return;
 
-  // Before tearing down the menu controller bridges, return the Cocoa menus to
-  // their initial state.
-  if (bookmarkMenuBridge_.get())
-    bookmarkMenuBridge_->ResetMenu();
-  if (historyMenuBridge_.get())
+  // Before tearing down the menu controller bridges, return the history menu to
+  // its initial state.
+  if (historyMenuBridge_)
     historyMenuBridge_->ResetMenu();
 
   // Rebuild the menus with the new profile.
   lastProfile_ = profile;
 
-  bookmarkMenuBridge_.reset(new BookmarkMenuBridge(lastProfile_,
-      [[[NSApp mainMenu] itemWithTag:IDC_BOOKMARKS_MENU] submenu]));
+  auto it = profileBookmarkMenuBridgeMap_.find(profile);
+  if (it == profileBookmarkMenuBridgeMap_.end()) {
+    base::scoped_nsobject<NSMenu> submenu(
+        [[[[NSApp mainMenu] itemWithTag:IDC_BOOKMARKS_MENU] submenu] copy]);
+    bookmarkMenuBridge_ = new BookmarkMenuBridge(lastProfile_, submenu);
+    profileBookmarkMenuBridgeMap_[profile] = bookmarkMenuBridge_;
+  } else {
+    bookmarkMenuBridge_ = it->second;
+  }
+
+  [[[NSApp mainMenu] itemWithTag:IDC_BOOKMARKS_MENU] setSubmenu:
+      bookmarkMenuBridge_->BookmarkMenu()];
   // No need to |BuildMenu| here.  It is done lazily upon menu access.
 
   historyMenuBridge_.reset(new HistoryMenuBridge(lastProfile_));
diff --git a/chrome/browser/app_controller_mac_browsertest.mm b/chrome/browser/app_controller_mac_browsertest.mm
index d79912e..a136a12 100644
--- a/chrome/browser/app_controller_mac_browsertest.mm
+++ b/chrome/browser/app_controller_mac_browsertest.mm
@@ -14,14 +14,19 @@
 #include "base/mac/scoped_nsobject.h"
 #include "base/prefs/pref_service.h"
 #include "base/run_loop.h"
+#include "base/strings/sys_string_conversions.h"
+#include "base/strings/utf_string_conversions.h"
 #include "chrome/app/chrome_command_ids.h"
+#include "components/bookmarks/browser/bookmark_model.h"
 #import "chrome/browser/app_controller_mac.h"
 #include "chrome/browser/apps/app_browsertest_util.h"
+#include "chrome/browser/bookmarks/bookmark_model_factory.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_list.h"
 #include "chrome/browser/ui/browser_window.h"
+#include "chrome/browser/ui/cocoa/bookmarks/bookmark_menu_bridge.h"
 #include "chrome/browser/ui/host_desktop.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/browser/ui/user_manager.h"
@@ -30,6 +35,7 @@
 #include "chrome/common/pref_names.h"
 #include "chrome/common/url_constants.h"
 #include "chrome/test/base/in_process_browser_test.h"
+#include "components/bookmarks/test/bookmark_test_helpers.h"
 #include "components/signin/core/common/profile_management_switches.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/test/test_navigation_observer.h"
@@ -412,4 +418,66 @@ IN_PROC_BROWSER_TEST_F(AppControllerReplaceNTPBrowserTest,
                 ->GetLastCommittedURL());
 }
 
+class AppControllerMainMenuBrowserTest : public InProcessBrowserTest {
+ protected:
+  AppControllerMainMenuBrowserTest() {
+  }
+};
+
+IN_PROC_BROWSER_TEST_F(AppControllerMainMenuBrowserTest,
+    BookmarksMenuIsRestoredAfterProfileSwitch) {
+  ProfileManager* profile_manager = g_browser_process->profile_manager();
+  base::scoped_nsobject<AppController> ac([[AppController alloc] init]);
+  [ac awakeFromNib];
+
+  // Constants for bookmarks that we will create later.
+  const base::string16 title1(base::ASCIIToUTF16("Dinosaur Comics"));
+  const GURL url1("http://qwantz.com//");
+
+  const base::string16 title2(base::ASCIIToUTF16("XKCD"));
+  const GURL url2("https://www.xkcd.com/");
+
+  // Use the existing profile as profile 1.
+  Profile* profile1 = browser()->profile();
+  bookmarks::test::WaitForBookmarkModelToLoad(
+      BookmarkModelFactory::GetForProfile(profile1));
+
+  // Create profile 2.
+  base::FilePath path2 = profile_manager->GenerateNextProfileDirectoryPath();
+  Profile* profile2 =
+      Profile::CreateProfile(path2, NULL, Profile::CREATE_MODE_SYNCHRONOUS);
+  profile_manager->RegisterTestingProfile(profile2, false, true);
+  bookmarks::test::WaitForBookmarkModelToLoad(
+      BookmarkModelFactory::GetForProfile(profile2));
+
+  // Switch to profile 1, create bookmark 1 and force the menu to build.
+  [ac windowChangedToProfile:profile1];
+  [ac bookmarkMenuBridge]->GetBookmarkModel()->AddURL(
+      [ac bookmarkMenuBridge]->GetBookmarkModel()->bookmark_bar_node(),
+      0, title1, url1);
+  [ac bookmarkMenuBridge]->BuildMenu();
+
+  // Switch to profile 2, create bookmark 2 and force the menu to build.
+  [ac windowChangedToProfile:profile2];
+  [ac bookmarkMenuBridge]->GetBookmarkModel()->AddURL(
+      [ac bookmarkMenuBridge]->GetBookmarkModel()->bookmark_bar_node(),
+      0, title2, url2);
+  [ac bookmarkMenuBridge]->BuildMenu();
+
+  // Test that only bookmark 2 is shown.
+  EXPECT_FALSE([[ac bookmarkMenuBridge]->BookmarkMenu() itemWithTitle:
+      SysUTF16ToNSString(title1)]);
+  EXPECT_TRUE([[ac bookmarkMenuBridge]->BookmarkMenu() itemWithTitle:
+      SysUTF16ToNSString(title2)]);
+
+  // Switch *back* to profile 1 and *don't* force the menu to build.
+  [ac windowChangedToProfile:profile1];
+
+  // Test that only bookmark 1 is shown in the restored menu.
+  EXPECT_TRUE([[ac bookmarkMenuBridge]->BookmarkMenu() itemWithTitle:
+      SysUTF16ToNSString(title1)]);
+  EXPECT_FALSE([[ac bookmarkMenuBridge]->BookmarkMenu() itemWithTitle:
+      SysUTF16ToNSString(title2)]);
+}
+
 }  // namespace
diff --git a/chrome/browser/ui/cocoa/bookmarks/bookmark_menu_bridge.h b/chrome/browser/ui/cocoa/bookmarks/bookmark_menu_bridge.h
index ac2032c..97dc4a1 100644
--- a/chrome/browser/ui/cocoa/bookmarks/bookmark_menu_bridge.h
+++ b/chrome/browser/ui/cocoa/bookmarks/bookmark_menu_bridge.h
@@ -78,6 +78,9 @@ class BookmarkMenuBridge : public BookmarkModelObserver,
   BookmarkModel* GetBookmarkModel();
   Profile* GetProfile();
 
+  // Return the Bookmark menu.
+  virtual NSMenu* BookmarkMenu();
+
  protected:
   // Rebuilds the bookmark content of supplied menu.
   void UpdateMenuInternal(NSMenu* bookmark_menu, bool is_submenu);
@@ -126,9 +129,6 @@ class BookmarkMenuBridge : public BookmarkModelObserver,
   // Returns the NSMenuItem for a given BookmarkNode.
   NSMenuItem* MenuItemForNode(const BookmarkNode* node);
 
-  // Return the Bookmark menu.
-  virtual NSMenu* BookmarkMenu();
-
   // Start watching the bookmarks for changes.
   void ObserveBookmarkModel();
 
diff --git a/chrome/browser/ui/cocoa/bookmarks/bookmark_menu_bridge.mm b/chrome/browser/ui/cocoa/bookmarks/bookmark_menu_bridge.mm
index ab763b0..194c94e 100644
--- a/chrome/browser/ui/cocoa/bookmarks/bookmark_menu_bridge.mm
+++ b/chrome/browser/ui/cocoa/bookmarks/bookmark_menu_bridge.mm
@@ -40,10 +40,6 @@ BookmarkMenuBridge::~BookmarkMenuBridge() {
   [controller_ release];
 }
 
-NSMenu* BookmarkMenuBridge::BookmarkMenu() {
-  return [controller_ menu];
-}
-
 void BookmarkMenuBridge::BookmarkModelLoaded(BookmarkModel* model,
                                              bool ids_reassigned) {
   InvalidateMenu();
@@ -202,6 +198,10 @@ Profile* BookmarkMenuBridge::GetProfile() {
   return profile_;
 }
 
+NSMenu* BookmarkMenuBridge::BookmarkMenu() {
+  return [controller_ menu];
+}
+
 void BookmarkMenuBridge::ClearBookmarkMenu(NSMenu* menu) {
   bookmark_nodes_.clear();
   // Recursively delete all menus that look like a bookmark. Also delete all
diff --git a/chrome/browser/ui/cocoa/bookmarks/bookmark_menu_cocoa_controller.h b/chrome/browser/ui/cocoa/bookmarks/bookmark_menu_cocoa_controller.h
index 2bd2335..3652a53 100644
--- a/chrome/browser/ui/cocoa/bookmarks/bookmark_menu_cocoa_controller.h
+++ b/chrome/browser/ui/cocoa/bookmarks/bookmark_menu_cocoa_controller.h
@@ -11,6 +11,7 @@
 
 #import <Cocoa/Cocoa.h>
 
+#import "base/mac/scoped_nsobject.h"
 #include "ui/base/window_open_disposition.h"
 
 class BookmarkNode;
@@ -19,7 +20,7 @@ class BookmarkMenuBridge;
 @interface BookmarkMenuCocoaController : NSObject<NSMenuDelegate> {
  @private
   BookmarkMenuBridge* bridge_;  // weak; owns me
-  NSMenu *menu_;
+  base::scoped_nsobject<NSMenu> menu_;
 }
 
 // The Bookmarks menu
diff --git a/chrome/browser/ui/cocoa/bookmarks/bookmark_menu_cocoa_controller.mm b/chrome/browser/ui/cocoa/bookmarks/bookmark_menu_cocoa_controller.mm
index 00f6447..010ec6a 100644
--- a/chrome/browser/ui/cocoa/bookmarks/bookmark_menu_cocoa_controller.mm
+++ b/chrome/browser/ui/cocoa/bookmarks/bookmark_menu_cocoa_controller.mm
@@ -51,7 +51,7 @@ const NSUInteger kMaximumMenuPixelsWide = 300;
   if ((self = [super init])) {
     bridge_ = bridge;
     DCHECK(bridge_);
-    menu_ = menu;
+    menu_.reset([menu retain]);
     [[self menu] setDelegate:self];
   }
   return self;