mash: Cleanup ash shelf application menu code.

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

 // Copyright 2013 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/chrome_launcher_controller_impl.h"
 
 #include <stddef.h>
 
 #include "ash/common/shelf/app_list_button.h"
 #include "ash/common/shelf/shelf_button.h"
@@ skipping 21 matching lines @@
 #include "chrome/browser/extensions/extension_apitest.h"
 #include "chrome/browser/extensions/extension_browsertest.h"
 #include "chrome/browser/extensions/extension_function_test_utils.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/launch_util.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/app_list/app_list_service.h"
 #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"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_commands.h"
 #include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/ui/browser_list.h"
 #include "chrome/browser/ui/browser_window.h"
 #include "chrome/browser/ui/chrome_pages.h"
 #include "chrome/browser/ui/extensions/app_launch_params.h"
@@ skipping 165 matching lines @@
   const ash::ShelfItem& GetLastLauncherPanelItem() {
     // Panels show up on the right side of the shelf, so the desired item
     // will be the last one.
     return shelf_model()->items()[shelf_model()->item_count() - 1];
   }
 
   LauncherItemController* GetItemController(ash::ShelfID id) {
     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_;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(LauncherPlatformAppBrowserTest);
 };
 
 enum RipOffCommand {
   // Drag the item off the shelf and let the mouse go.
   RIP_OFF_ITEM,
@@ skipping 20 matching lines @@
         ash::WmShelf::ForWindow(ash::WmShell::Get()->GetPrimaryRootWindow());
     model_ = ash::WmShell::Get()->shelf_model();
     controller_ = GetChromeLauncherControllerImpl();
     ASSERT_TRUE(controller_);
     return ExtensionBrowserTest::RunTestOnMainThreadLoop();
   }
 
   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(
       const char* name,
       extensions::LaunchContainer container,
       WindowOpenDisposition disposition) {
     EXPECT_TRUE(LoadExtension(test_data_dir_.AppendASCII(name)));
 
     ExtensionService* service = extensions::ExtensionSystem::Get(
         profile())->extension_service();
@@ skipping 249 matching lines @@
   // Then close it, make sure the item's gone.
   CloseAppWindow(window);
   --item_count;
   ASSERT_EQ(item_count, shelf_model()->item_count());
 }
 
 // Test that we can launch a platform app with more than one window.
 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());
 }
 
 IN_PROC_BROWSER_TEST_F(LauncherPlatformAppBrowserTest, MultipleApps) {
   int item_count = shelf_model()->item_count();
 
   // First run app.
   const Extension* extension1 = LoadAndLaunchPlatformApp("launch", "Launched");
   AppWindow* window1 = CreateAppWindow(browser()->profile(), extension1);
   ++item_count;
@@ skipping 1817 matching lines @@
 
   // Close all windows via the menu item.
   CloseBrowserWindow(browser(), menu1.get(), LauncherContextMenu::MENU_CLOSE);
   EXPECT_EQ(0u, BrowserList::GetInstance()->size());
 
   // Check if "Close" is removed from the context menu.
   std::unique_ptr<LauncherContextMenu> menu2 = CreateBrowserItemContextMenu();
   ASSERT_FALSE(
       IsItemPresentInMenu(menu2.get(), LauncherContextMenu::MENU_CLOSE));
 }