mash: Cleanup ash shelf application menu code.

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

Index: chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_browsertest.cc
diff --git a/chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_browsertest.cc b/chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_browsertest.cc
index 0f1b8e346099e5f37ef7a29918c9439e8477d615..0bc5eee70ac6663d1f2d7ce9a15685b6621ee93a 100644
--- a/chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_browsertest.cc
+++ b/chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_browsertest.cc
@@ -39,7 +39,6 @@
 #include "chrome/browser/ui/ash/app_list/test/app_list_service_ash_test_api.h"
 #include "chrome/browser/ui/ash/launcher/browser_shortcut_launcher_item_controller.h"
 #include "chrome/browser/ui/ash/launcher/chrome_launcher_controller_util.h"
-#include "chrome/browser/ui/ash/launcher/launcher_application_menu_item_model.h"
 #include "chrome/browser/ui/ash/launcher/launcher_context_menu.h"
 #include "chrome/browser/ui/ash/launcher/launcher_item_controller.h"
 #include "chrome/browser/ui/ash/session_controller_client.h"
@@ -225,18 +224,9 @@ class LauncherPlatformAppBrowserTest
     return controller_->GetLauncherItemController(id);
   }
 
-  // Returns the number of menu items, ignoring separators.
+  // Returns the number of application menu items, ignoring separators.
   int GetNumApplicationMenuItems(const ash::ShelfItem& item) {
-    const int event_flags = 0;
-    std::unique_ptr<ui::SimpleMenuModel> menu(
-        new LauncherApplicationMenuItemModel(
-            controller_->GetApplicationList(item, event_flags)));
-    int num_items = 0;
-    for (int i = 0; i < menu->GetItemCount(); ++i) {
-      if (menu->GetTypeAt(i) != ui::MenuModel::TYPE_SEPARATOR)
-        ++num_items;
-    }
-    return num_items;
+    return controller_->GetAppMenuItems(item, 0 /* event_flags */).size();

[James Cook, 2017/02/06 17:30:21]

int / size_t conversion?

[msw, 2017/02/07 09:12:01]

Ah, I already inlined this functionality; I have now removed this method.

   }

[James Cook, 2017/02/06 17:30:22]

btw, I really like how you made GetAppMenuItems() just return real items and not
mix up the data model with separators.

[msw, 2017/02/07 09:12:01]

Acknowledged; but client-specified menu item lists may likely need to support
listing separators. ash::mojom::ContextMenuItem::Type already has SEPARATOR.

 
   ChromeLauncherControllerImpl* controller_;
@@ -278,10 +268,9 @@ class ShelfAppBrowserTest : public ExtensionBrowserTest {
   size_t NumberOfDetectedLauncherBrowsers(bool show_all_tabs) {
     LauncherItemController* item_controller =
       controller_->GetBrowserShortcutLauncherItemController();
-    int items = item_controller->GetApplicationList(
-        show_all_tabs ? ui::EF_SHIFT_DOWN : 0).size();
-    // If we have at least one item, we have also a title which we remove here.
-    return items ? (items - 1) : 0;
+    return item_controller
+        ->GetAppMenuItems(show_all_tabs ? ui::EF_SHIFT_DOWN : 0)
+        .size();
   }
 
   const Extension* LoadAndLaunchExtension(
@@ -551,37 +540,32 @@ IN_PROC_BROWSER_TEST_F(LauncherPlatformAppBrowserTest, UnpinRunning) {
 IN_PROC_BROWSER_TEST_F(LauncherPlatformAppBrowserTest, MultipleWindows) {
   int item_count = shelf_model()->item_count();
 
-  // First run app.
+  // Run the application; a shelf item should be added with one app menu item.
   const Extension* extension = LoadAndLaunchPlatformApp("launch", "Launched");
   AppWindow* window1 = CreateAppWindow(browser()->profile(), extension);
-  ++item_count;
-  ASSERT_EQ(item_count, shelf_model()->item_count());
+  ASSERT_EQ(item_count + 1, shelf_model()->item_count());
   const ash::ShelfItem& item1 = GetLastLauncherItem();
   ash::ShelfID item_id = item1.id;
   EXPECT_EQ(ash::TYPE_APP, item1.type);
   EXPECT_EQ(ash::STATUS_ACTIVE, item1.status);
-  EXPECT_EQ(2, GetNumApplicationMenuItems(item1));  // Title + 1 window
+  EXPECT_EQ(1U, controller_->GetAppMenuItems(item1, 0).size());  // 1 window

[James Cook, 2017/02/06 17:30:21]

super nit question: Do we prefer 1U or 1u ?

[msw, 2017/02/07 09:12:01]

Hmm, my grep foo is weak at the moment, so I don't have satisfying results, but
I support lowercase (it does seem more visibly distinct from numerals, which may
make the number values easier to read?); I changed these.
https://cs.chromium.org/search/?q=%5B0-9%5DU%5B%5Ea-zA-Z0-9%5D&type=cs

 
-  // Add second window.
+  // Add a second window; confirm the shelf item stays; check the app menu.
   AppWindow* window2 = CreateAppWindow(browser()->profile(), extension);
-  // Confirm item stays.
-  ASSERT_EQ(item_count, shelf_model()->item_count());
+  ASSERT_EQ(item_count + 1, shelf_model()->item_count());
   const ash::ShelfItem& item2 = *shelf_model()->ItemByID(item_id);
   EXPECT_EQ(ash::STATUS_ACTIVE, item2.status);
-  EXPECT_EQ(3, GetNumApplicationMenuItems(item2));  // Title + 2 windows
+  EXPECT_EQ(2U, controller_->GetAppMenuItems(item2, 0).size());  // 2 windows
 
-  // Close second window.
+  // Close the second window; confirm the shelf item stays; check the app menu.
   CloseAppWindow(window2);
-  // Confirm item stays.
-  ASSERT_EQ(item_count, shelf_model()->item_count());
+  ASSERT_EQ(item_count + 1, shelf_model()->item_count());
   const ash::ShelfItem& item3 = *shelf_model()->ItemByID(item_id);
   EXPECT_EQ(ash::STATUS_ACTIVE, item3.status);
-  EXPECT_EQ(2, GetNumApplicationMenuItems(item3));  // Title + 1 window
+  EXPECT_EQ(1U, controller_->GetAppMenuItems(item3, 0).size());  // 1 window
 
-  // Close first window.
+  // Close the first window; the shelf item should be removed.
   CloseAppWindow(window1);
-  // Confirm item is removed.
-  --item_count;
   ASSERT_EQ(item_count, shelf_model()->item_count());
 }