mash: Remove shelf app menu item objects.

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

Index: chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.cc
diff --git a/chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.cc b/chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.cc
index b42150b006eb36b65525068618181486b95dcacd..d2312c50c01790ee70a67685dea22390d0bcd6bb 100644
--- a/chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.cc
+++ b/chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.cc
@@ -13,7 +13,6 @@
 #include "chrome/browser/extensions/launch_util.h"
 #include "chrome/browser/ui/app_list/arc/arc_app_utils.h"
 #include "chrome/browser/ui/ash/launcher/arc_playstore_shortcut_launcher_item_controller.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"
@@ -126,17 +125,42 @@ ash::ShelfAction AppShortcutLauncherItemController::ItemSelected(
 ash::ShelfAppMenuItemList AppShortcutLauncherItemController::GetAppMenuItems(
     int event_flags) {
   ash::ShelfAppMenuItemList items;
-  std::vector<content::WebContents*> content_list = GetRunningApplications();
-  for (size_t i = 0; i < content_list.size(); i++) {
-    content::WebContents* web_contents = content_list[i];
-    gfx::Image app_icon = launcher_controller()->GetAppListIcon(web_contents);
+  app_menu_items_ = GetRunningApplications();
+  CHECK_LT(app_menu_items_.size(), std::numeric_limits<uint32_t>::max());

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

Is this useful? It looks like GetRunningApplications() always returns a small
positive integer.

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

These checks are indeed a bit paranoid... I doubt we'll ever get so many tabs
per window, browser windows, or app windows as to break these menus. Still, I
figured that explicitly checking (and clarifying the menu's capabilities) is
better than failing in some surprising way later on, but I'll remove this and
other checks if you prefer.

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

This check implies GetRunningApplications() can loop 4 billion (minus 1) times.
If you're truly concerned about GetRunningApplications() going wild it should
early exit.

These lists end up being menus, which can probably show a max of ~40 items even
on a 30 inch monitor.

Thoughts:
* Check vs. uint16_t (is that what you meant initially?)
* or Check vs. uint8_t
* and/or Early exit loop below

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

I was checking against uint32_t, because that's the extent of the command id
space, and I'm casting a potentially larger size_t value to that (see checked
cast below). The browser shortcut commands split that space for window and tab
ids (hence uint16_t there). I guess this might be more confusing than useful, so
I removed the check and others like it; the checked_cast should suffice.

+  for (size_t i = 0; i < app_menu_items_.size(); i++) {
+    content::WebContents* web_contents = app_menu_items_[i];
+    gfx::Image 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));
+    items.push_back(base::MakeUnique<ash::ShelfApplicationMenuItem>(
+        base::checked_cast<uint32_t>(i), title, &icon));
   }
   return items;
 }
 
+void AppShortcutLauncherItemController::ExecuteCommand(uint32_t command_id,
+                                                       int32_t event_flags) {
+  if (static_cast<size_t>(command_id) >= app_menu_items_.size())
+    return;
+
+  content::WebContents* web_contents = app_menu_items_[command_id];
+  Browser* browser = chrome::FindBrowserWithWebContents(web_contents);
+  if (!browser)
+    return;
+
+  TabStripModel* tab_strip = browser->tab_strip_model();
+  int index = tab_strip->GetIndexOfWebContents(web_contents);
+  DCHECK_NE(index, TabStripModel::kNoTab);

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

Should this early-exit? Is it possible a tab could close while the menu is open?

Also, I think this function could use a comment about the lifetime of
web_contents pointers vs. them existing in app_menu_items_. For example, if a
web contents goes away do we still cache an invalid pointer to it?

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

Done.

+  if (event_flags & (ui::EF_SHIFT_DOWN | ui::EF_MIDDLE_MOUSE_BUTTON)) {
+    tab_strip->CloseWebContentsAt(index, TabStripModel::CLOSE_USER_GESTURE);
+  } else {
+    multi_user_util::MoveWindowToCurrentDesktop(
+        browser->window()->GetNativeWindow());
+    tab_strip->ActivateTabAt(index, false);
+    browser->window()->Show();
+    browser->window()->Activate();
+  }
+}
+
 void AppShortcutLauncherItemController::Close() {
   // Close all running 'programs' of this type.
   std::vector<content::WebContents*> content =