mash: Remove shelf app menu item objects.

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

 // Copyright (c) 2012 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.
 
 #include "chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.h"
 
 #include <stddef.h>
 
 #include "ash/public/cpp/shelf_application_menu_item.h"
 #include "ash/wm/window_util.h"
 #include "base/memory/ptr_util.h"
 #include "chrome/browser/chromeos/arc/arc_support_host.h"
 #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"
 #include "chrome/browser/ui/ash/launcher/launcher_controller_helper.h"
 #include "chrome/browser/ui/ash/launcher/launcher_item_controller.h"
 #include "chrome/browser/ui/ash/multi_user/multi_user_util.h"
 #include "chrome/browser/ui/ash/multi_user/multi_user_window_manager.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/ui/browser_list.h"
@@ skipping 92 matching lines @@
     launcher_controller()->LaunchApp(ash::AppLauncherId(app_id(), launch_id()),
                                      source, ui::EF_NONE);
     return ash::SHELF_ACTION_NEW_WINDOW_CREATED;
   }
   return ActivateContent(content);
 }
 
 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 =
       launcher_controller()->GetV1ApplicationsFromAppId(app_id());
   for (size_t i = 0; i < content.size(); i++) {
     Browser* browser = chrome::FindBrowserWithWebContents(content[i]);
     if (!browser || !IsBrowserFromActiveUser(browser))
       continue;
     TabStripModel* tab_strip = browser->tab_strip_model();
     int index = tab_strip->GetIndexOfWebContents(content[i]);
@@ skipping 173 matching lines @@
 
 bool AppShortcutLauncherItemController::AllowNextLaunchAttempt() {
   if (last_launch_attempt_.is_null() ||
       last_launch_attempt_ + base::TimeDelta::FromMilliseconds(
           kClickSuppressionInMS) < base::Time::Now()) {
     last_launch_attempt_ = base::Time::Now();
     return true;
   }
   return false;
 }