mash: Remove shelf app menu item objects.

chrome/browser/ui/ash/launcher/browser_shortcut_launcher_item_controller.cc

Index: chrome/browser/ui/ash/launcher/browser_shortcut_launcher_item_controller.cc
diff --git a/chrome/browser/ui/ash/launcher/browser_shortcut_launcher_item_controller.cc b/chrome/browser/ui/ash/launcher/browser_shortcut_launcher_item_controller.cc
index 5e043d9260d3db65460162063b25622c24203ac6..0ee59dd43570ad361960b59e443d5957e5331860 100644
--- a/chrome/browser/ui/ash/launcher/browser_shortcut_launcher_item_controller.cc
+++ b/chrome/browser/ui/ash/launcher/browser_shortcut_launcher_item_controller.cc
@@ -4,6 +4,7 @@
 
 #include "chrome/browser/ui/ash/launcher/browser_shortcut_launcher_item_controller.h"
 
+#include <limits>
 #include <vector>
 
 #include "ash/common/shelf/shelf_delegate.h"
@@ -15,12 +16,12 @@
 #include "ash/resources/grit/ash_resources.h"
 #include "ash/wm/window_util.h"
 #include "base/memory/ptr_util.h"
+#include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/profiles/profile.h"
-#include "chrome/browser/ui/ash/launcher/chrome_launcher_app_menu_item_browser.h"
-#include "chrome/browser/ui/ash/launcher/chrome_launcher_app_menu_item_tab.h"
 #include "chrome/browser/ui/ash/launcher/chrome_launcher_controller.h"
 #include "chrome/browser/ui/ash/launcher/chrome_launcher_controller_util.h"
 #include "chrome/browser/ui/ash/launcher/launcher_context_menu.h"
+#include "chrome/browser/ui/ash/multi_user/multi_user_util.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_commands.h"
 #include "chrome/browser/ui/browser_finder.h"
@@ -33,17 +34,23 @@
 #include "chrome/grit/chromium_strings.h"
 #include "chrome/grit/generated_resources.h"
 #include "components/strings/grit/components_strings.h"
+#include "content/public/browser/notification_service.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/url_constants.h"
 #include "ui/aura/window.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/events/event.h"
+#include "ui/events/event_constants.h"
 #include "ui/gfx/image/image.h"
 #include "ui/wm/core/window_animations.h"
 
 namespace {
 
+// The maximum number of browser or tab items supported in the application menu.
+// Also used as a tab-index flag for browser-specific commands that ignore tabs.

[James Cook, 2017/03/01 19:50:48]

It might be nice to have a separate constant for this, like kAllTabs or kNoTabs.
I got confused below with some of the uses of kMaxItems.

[msw, 2017/03/02 02:59:20]

Done.

+const uint16_t kMaxItems = std::numeric_limits<uint16_t>::max();
+
 bool IsSettingsBrowser(Browser* browser) {
   // Normally this test is sufficient. TODO(stevenjb): Replace this with a
   // better mechanism (Settings WebUI or Browser type).
@@ -58,6 +65,44 @@ bool IsSettingsBrowser(Browser* browser) {
   return false;
 }
 
+// Returns a 32-bit command id from 16-bit browser and web-contents indices.
+uint32_t GetCommandId(uint16_t browser_index, uint16_t web_contents_index) {
+  return (browser_index << 16) | web_contents_index;
+}
+
+// Get the 16-bit browser index from a 32-bit command id.
+uint16_t GetBrowserIndex(uint32_t command_id) {
+  return base::checked_cast<uint16_t>((command_id >> 16) & 0xFFFF);
+}
+
+// Get the 16-bit web-contents index from a 32-bit command id.
+uint16_t GetWebContentsIndex(uint32_t command_id) {
+  return base::checked_cast<uint16_t>(command_id & 0xFFFF);
+}
+
+// Check if the given |web_contents| is in incognito mode.
+bool IsIncognito(content::WebContents* web_contents) {
+  const Profile* profile =
+      Profile::FromBrowserContext(web_contents->GetBrowserContext());
+  return profile->IsOffTheRecord() && !profile->IsGuestSession();
+}
+
+// Get the favicon for the browser list entry for |web_contents|.
+// Note that for incognito windows the incognito icon will be returned.
+gfx::Image GetBrowserListIcon(content::WebContents* web_contents) {
+  ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
+  return rb.GetImageNamed(IsIncognito(web_contents)
+                              ? IDR_ASH_SHELF_LIST_INCOGNITO_BROWSER
+                              : IDR_ASH_SHELF_LIST_BROWSER);
+}
+
+// Get the title for the browser list entry for |web_contents|.
+// If |web_contents| has not loaded, returns "New Tab".
+base::string16 GetBrowserListTitle(content::WebContents* web_contents) {
+  const base::string16& title = web_contents->GetTitle();
+  return title.empty() ? l10n_util::GetStringUTF16(IDS_NEW_TAB_TITLE) : title;
+}
+
 }  // namespace
 
 BrowserShortcutLauncherItemController::BrowserShortcutLauncherItemController(
@@ -69,7 +114,9 @@ BrowserShortcutLauncherItemController::BrowserShortcutLauncherItemController(
       shelf_model_(shelf_model) {}
 
 BrowserShortcutLauncherItemController::
-    ~BrowserShortcutLauncherItemController() {}
+    ~BrowserShortcutLauncherItemController() {
+  registrar_.RemoveAll();

[James Cook, 2017/03/01 19:50:48]

This isn't handled automatically by the registrar's destructor? If there's an
ordering issue that deserves a comment.

[msw, 2017/03/02 02:59:20]

You're right, it's automatically handled; I removed this.

+}
 
 void BrowserShortcutLauncherItemController::UpdateBrowserItemState() {
   // Determine the new browser's active state and change if necessary.
@@ -157,41 +204,80 @@ ash::ShelfAction BrowserShortcutLauncherItemController::ItemSelected(
 
 ash::ShelfAppMenuItemList
 BrowserShortcutLauncherItemController::GetAppMenuItems(int event_flags) {
+  browser_menu_items_.clear();
+  registrar_.RemoveAll();
+
   ash::ShelfAppMenuItemList items;
   bool found_tabbed_browser = false;
   for (auto* browser : GetListOfActiveBrowsers()) {
+    if (browser_menu_items_.size() >= kMaxItems)

[James Cook, 2017/03/01 19:50:48]

LOL. I'd hate to meet this power-user. :-)

[msw, 2017/03/02 02:59:20]

Haha, yeah; I'm not sure what would break first if a user tried to have 65k+
windows/tabs open. Sadly, this menu won't fit their needs.

+      break;
     TabStripModel* tab_strip = browser->tab_strip_model();
-    if (tab_strip->active_index() == -1)
+    const int tab_index = tab_strip->active_index();
+    if (tab_index < 0 || tab_index >= kMaxItems)
       continue;
     if (browser->is_type_tabbed())
       found_tabbed_browser = true;
     if (!(event_flags & ui::EF_SHIFT_DOWN)) {
-      content::WebContents* web_contents =
-          tab_strip->GetWebContentsAt(tab_strip->active_index());
-      gfx::Image app_icon = GetBrowserListIcon(web_contents);
-      base::string16 title = GetBrowserListTitle(web_contents);
-      items.push_back(base::MakeUnique<ChromeLauncherAppMenuItemBrowser>(
-          title, &app_icon, browser));
+      content::WebContents* tab = tab_strip->GetWebContentsAt(tab_index);

[James Cook, 2017/03/01 19:50:48]

I like "tab" -- briefer and clearer!

[msw, 2017/03/02 02:59:20]

Acknowledged.

+      gfx::Image icon = GetBrowserListIcon(tab);
+      base::string16 title = GetBrowserListTitle(tab);
+      items.push_back(base::MakeUnique<ash::ShelfApplicationMenuItem>(
+          GetCommandId(browser_menu_items_.size(), kMaxItems), title, &icon));
     } else {
-      for (int index = 0; index  < tab_strip->count(); ++index) {
-        content::WebContents* web_contents =
-            tab_strip->GetWebContentsAt(index);
-        gfx::Image app_icon =
-            launcher_controller()->GetAppListIcon(web_contents);
-        base::string16 title =
-            launcher_controller()->GetAppListTitle(web_contents);
-        items.push_back(base::MakeUnique<ChromeLauncherAppMenuItemTab>(
-            title, &app_icon, web_contents));
+      for (uint16_t i = 0; i < tab_strip->count() && i < kMaxItems; ++i) {
+        content::WebContents* tab = tab_strip->GetWebContentsAt(i);
+        gfx::Image icon = launcher_controller()->GetAppListIcon(tab);
+        base::string16 title = launcher_controller()->GetAppListTitle(tab);
+        items.push_back(base::MakeUnique<ash::ShelfApplicationMenuItem>(
+            GetCommandId(browser_menu_items_.size(), i), title, &icon));
       }
     }
+    browser_menu_items_.push_back(browser);
+    registrar_.Add(this, chrome::NOTIFICATION_BROWSER_CLOSING,
+                   content::Source<Browser>(browser));
   }
   // If only windowed applications are open, we return an empty list to
   // enforce the creation of a new browser.
-  if (!found_tabbed_browser)
+  if (!found_tabbed_browser) {
     items.clear();
+    browser_menu_items_.clear();
+    registrar_.RemoveAll();
+  }
   return items;
 }
 
+void BrowserShortcutLauncherItemController::ExecuteCommand(
+    uint32_t command_id,
+    int32_t event_flags) {
+  const uint16_t browser_index = GetBrowserIndex(command_id);
+  if (browser_index >= browser_menu_items_.size())
+    return;
+
+  // The indicated browser may have closed while the menu was open.
+  Browser* browser = browser_menu_items_[browser_index];
+  if (!browser)
+    return;
+
+  TabStripModel* tab_strip = browser->tab_strip_model();
+  const uint16_t tab_index = GetWebContentsIndex(command_id);
+  if (event_flags & (ui::EF_SHIFT_DOWN | ui::EF_MIDDLE_MOUSE_BUTTON)) {
+    if (tab_index == kMaxItems) {
+      tab_strip->CloseAllTabs();
+    } else if (tab_strip->ContainsIndex(tab_index)) {
+      tab_strip->CloseWebContentsAt(tab_index,
+                                    TabStripModel::CLOSE_USER_GESTURE);
+    }
+  } else {
+    multi_user_util::MoveWindowToCurrentDesktop(
+        browser->window()->GetNativeWindow());
+    if (tab_index != kMaxItems && tab_strip->ContainsIndex(tab_index))
+      tab_strip->ActivateTabAt(tab_index, false);
+    browser->window()->Show();
+    browser->window()->Activate();
+  }
+}

[James Cook, 2017/03/01 19:50:48]

Should this wipe the items and registrar?

[msw, 2017/03/02 02:59:20]

It doesn't seem too important either way, and I'd have to restructure the code
to do |browser_menu_items_.clear();| and |registrar_.RemoveAll();| in the early
return cases too. I left this and the other menu item arrays intact after
ExecuteCommand; let me know if your preference is to clear them.

[James Cook, 2017/03/02 15:43:00]

Does this controller object get deleted when the menu closes? I'm guessing not.

If not, I think we should wipe the array (and especially the notification
listeners).

[msw, 2017/03/02 18:37:47]

Done. (here and for app shortcuts' cached menu item list)

+
 void BrowserShortcutLauncherItemController::Close() {
   for (auto* browser : GetListOfActiveBrowsers())
     browser->window()->Close();
@@ -201,29 +287,6 @@ bool BrowserShortcutLauncherItemController::IsListOfActiveBrowserEmpty() {
   return GetListOfActiveBrowsers().empty();
 }
 
-gfx::Image BrowserShortcutLauncherItemController::GetBrowserListIcon(
-    content::WebContents* web_contents) const {
-  ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
-  return rb.GetImageNamed(IsIncognito(web_contents) ?
-      IDR_ASH_SHELF_LIST_INCOGNITO_BROWSER :
-      IDR_ASH_SHELF_LIST_BROWSER);
-}
-
-base::string16 BrowserShortcutLauncherItemController::GetBrowserListTitle(
-    content::WebContents* web_contents) const {
-  base::string16 title = web_contents->GetTitle();
-  if (!title.empty())
-    return title;
-  return l10n_util::GetStringUTF16(IDS_NEW_TAB_TITLE);
-}
-
-bool BrowserShortcutLauncherItemController::IsIncognito(
-    content::WebContents* web_contents) const {
-  const Profile* profile =
-      Profile::FromBrowserContext(web_contents->GetBrowserContext());
-  return profile->IsOffTheRecord() && !profile->IsGuestSession();
-}
-
 ash::ShelfAction
 BrowserShortcutLauncherItemController::ActivateOrAdvanceToNextBrowser() {
   // Create a list of all suitable running browsers.
@@ -311,3 +374,19 @@ BrowserShortcutLauncherItemController::GetListOfActiveBrowsers() {
   }
   return active_browsers;
 }
+
+void BrowserShortcutLauncherItemController::Observe(
+    int type,
+    const content::NotificationSource& source,
+    const content::NotificationDetails& details) {
+  DCHECK_EQ(chrome::NOTIFICATION_BROWSER_CLOSING, type);
+  Browser* browser = content::Source<Browser>(source).ptr();
+  DCHECK(browser);
+  BrowserList::BrowserVector::iterator item = std::find(
+      browser_menu_items_.begin(), browser_menu_items_.end(), browser);
+  DCHECK(item != browser_menu_items_.end());
+  // Clear the entry for the closed browser and leave other indices intact.
+  *item = nullptr;
+  registrar_.Remove(this, chrome::NOTIFICATION_BROWSER_CLOSING,
+                    content::Source<Browser>(browser));
+}