mash: Remove ChromeLauncherController's |id_to_item_controller_map_|.

Remove CLC::id_to_item_controller_map_; rely on ShelfModel instead.
Skip removing shelf items in CLC destruction; rely on ~ShelfModel instead.

Remove CLC::[Get|Set]ShelfItemDelegate helpers; inline ShelfModel calls.
Remove moot ShelfModelObserver::OnSetShelfItemDelegate.

Rename CLC::LauncherItemClosed -> RemoveShelfItem.
Rename test helper GetAppLaunchers -> GetPinnedAppIds.

Inline ShelfModel::RemoveShelfItemDelegate helper function.
Avoid GetItemIndexForType, use GetShelfIDForAppID instead.
Use ItemTypeIsPinned helper function.

BUG=557406
TEST=Automated; no Chrome OS shelf behavior changes.
R=jamescook@chromium.org

Review-Url: https://codereview.chromium.org/2798173002
Cr-Commit-Position: refs/heads/master@{#463030}

ash/common/shelf/shelf_model.cc

@@ -70,10 +70,7 @@ void ShelfModel::RemoveItemAt(int index) {
   DCHECK(index >= 0 && index < item_count());
   ShelfItem old_item(items_[index]);
   items_.erase(items_.begin() + index);
-  RemoveShelfItemDelegate(old_item.id);
-  // TODO(jamescook): Fold this into ShelfItemRemoved in existing observers.
-  for (auto& observer : observers_)
-    observer.OnSetShelfItemDelegate(old_item.id, nullptr);
+  id_to_item_delegate_map_.erase(old_item.id);
   for (auto& observer : observers_)
     observer.ShelfItemRemoved(index, old_item);
 }
@@ -160,14 +157,9 @@ int ShelfModel::FirstPanelIndex() const {
 void ShelfModel::SetShelfItemDelegate(
     ShelfID id,
     std::unique_ptr<ShelfItemDelegate> item_delegate) {
-  // If another ShelfItemDelegate is already registered for |id|, we assume
-  // that this request is replacing ShelfItemDelegate for |id| with
-  // |item_delegate|.
-  RemoveShelfItemDelegate(id);
-
-  for (auto& observer : observers_)
-    observer.OnSetShelfItemDelegate(id, item_delegate.get());
-
+  if (item_delegate)
+    item_delegate->set_shelf_id(id);
+  // This assignment replaces any ShelfItemDelegate already registered for |id|.
   id_to_item_delegate_map_[id] = std::move(item_delegate);
 }
 
@@ -203,9 +195,4 @@ int ShelfModel::ValidateInsertionIndex(ShelfItemType type, int index) const {
   return index;
 }
 
-void ShelfModel::RemoveShelfItemDelegate(ShelfID id) {
-  if (id_to_item_delegate_map_.find(id) != id_to_item_delegate_map_.end())
-    id_to_item_delegate_map_.erase(id);
-}
-
 }  // namespace ash

ash/common/shelf/shelf_model_observer.h

@@ -11,7 +11,6 @@
 namespace ash {
 
 struct ShelfItem;
-class ShelfItemDelegate;
 
 class ASH_EXPORT ShelfModelObserver {
  public:
@@ -26,17 +25,9 @@ class ASH_EXPORT ShelfModelObserver {
   // of the arguments.
   virtual void ShelfItemMoved(int start_index, int target_index) = 0;
 
-  // Invoked when the state of an item changes. |old_item| is the item
-  // before the change.
+  // Invoked after an item changes. |old_item| is the item before the change.
   virtual void ShelfItemChanged(int index, const ShelfItem& old_item) = 0;
 
-  // Gets called when a ShelfItemDelegate gets changed. Note that
-  // |item_delegate| can be null.
-  // NOTE: This is added a temporary fix for M39 to fix crbug.com/429870.
-  // TODO(skuhne): Find the real reason for this problem and remove this fix.
-  virtual void OnSetShelfItemDelegate(ShelfID id,
-                                      ShelfItemDelegate* item_delegate) = 0;
-
  protected:
   virtual ~ShelfModelObserver() {}
 };

ash/common/shelf/shelf_model_unittest.cc

@@ -37,7 +37,6 @@ class TestShelfModelObserver : public ShelfModelObserver {
   void ShelfItemRemoved(int, const ShelfItem&) override { removed_count_++; }
   void ShelfItemChanged(int, const ShelfItem&) override { changed_count_++; }
   void ShelfItemMoved(int, int) override { moved_count_++; }
-  void OnSetShelfItemDelegate(ShelfID, ShelfItemDelegate*) override {}
 
  private:
   void AddToResult(const std::string& format, int count, std::string* result) {

ash/common/shelf/shelf_view.cc

@@ -1591,8 +1591,6 @@ void ShelfView::ShelfItemMoved(int start_index, int target_index) {
     AnimateToIdealBounds();
 }
 
-void ShelfView::OnSetShelfItemDelegate(ShelfID, ShelfItemDelegate*) {}
-
 void ShelfView::AfterItemSelected(
     const ShelfItem& item,
     views::Button* sender,

ash/common/shelf/shelf_view.h

@@ -289,7 +289,6 @@ class ASH_EXPORT ShelfView : public views::View,
   void ShelfItemRemoved(int model_index, const ShelfItem& old_item) override;
   void ShelfItemChanged(int model_index, const ShelfItem& old_item) override;
   void ShelfItemMoved(int start_index, int target_index) override;
-  void OnSetShelfItemDelegate(ShelfID, ShelfItemDelegate*) override;
 
   // Handles the result of an item selection, records the |action| taken and
   // optionally shows an application menu with the given |menu_items|.

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

@@ -4,6 +4,8 @@
 
 #include "chrome/browser/ui/ash/launcher/arc_app_deferred_launcher_controller.h"
 
+#include "ash/common/shelf/shelf_model.h"
+#include "ash/shell.h"
 #include "base/memory/ptr_util.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "chrome/browser/chromeos/arc/arc_util.h"
@@ -111,8 +113,9 @@ void ArcAppDeferredLauncherController::Close(const std::string& app_id) {
     return;
 
   const ash::ShelfID shelf_id = owner_->GetShelfIDForAppID(shelf_app_id);
+  ash::ShelfModel* shelf_model = ash::Shell::Get()->shelf_model();
   const bool need_close_item =
-      it->second == owner_->GetShelfItemDelegate(shelf_id);
+      it->second == shelf_model->GetShelfItemDelegate(shelf_id);
   app_controller_map_.erase(it);
   if (need_close_item)
     owner_->CloseLauncherItem(shelf_id);
@@ -210,7 +213,8 @@ void ArcAppDeferredLauncherController::RegisterDeferredLaunch(
   if (shelf_id == 0) {
     owner_->CreateAppLauncherItem(std::move(controller), ash::STATUS_RUNNING);
   } else {
-    owner_->SetShelfItemDelegate(shelf_id, std::move(controller));
+    ash::ShelfModel* shelf_model = ash::Shell::Get()->shelf_model();
+    shelf_model->SetShelfItemDelegate(shelf_id, std::move(controller));
     owner_->SetItemStatus(shelf_id, ash::STATUS_RUNNING);
   }
 

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

@@ -3,6 +3,7 @@
 // found in the LICENSE file.
 
 #include "ash/common/shelf/shelf_delegate.h"
+#include "ash/common/shelf/shelf_model.h"
 #include "ash/public/cpp/shelf_item_delegate.h"
 #include "ash/shell.h"
 #include "ash/wm/window_util.h"
@@ -259,9 +260,7 @@ class ArcAppLauncherBrowserTest : public ExtensionBrowserTest {
 
   ash::ShelfItemDelegate* GetAppItemController(const std::string& id) {
     const ash::ShelfID shelf_id = shelf_delegate()->GetShelfIDForAppID(id);
-    if (!shelf_id)
-      return nullptr;
-    return chrome_controller()->GetShelfItemDelegate(shelf_id);
+    return ash::Shell::Get()->shelf_model()->GetShelfItemDelegate(shelf_id);
   }
 
   ArcAppListPrefs* app_prefs() { return ArcAppListPrefs::Get(profile()); }

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

@@ -6,6 +6,7 @@
 #include <string>
 
 #include "ash/common/shelf/shelf_delegate.h"
+#include "ash/common/shelf/shelf_model.h"
 #include "ash/common/wm/maximize_mode/maximize_mode_controller.h"
 #include "ash/common/wm/window_state.h"
 #include "ash/common/wm_window.h"
@@ -659,7 +660,8 @@ ArcAppWindowLauncherController::AttachControllerToTask(
   if (!shelf_id) {
     owner()->CreateAppLauncherItem(std::move(controller), ash::STATUS_RUNNING);
   } else {
-    owner()->SetShelfItemDelegate(shelf_id, std::move(controller));
+    ash::ShelfModel* shelf_model = ash::Shell::Get()->shelf_model();
+    shelf_model->SetShelfItemDelegate(shelf_id, std::move(controller));
     owner()->SetItemStatus(shelf_id, ash::STATUS_RUNNING);
   }
   item_controller->AddTaskId(task_id);

chrome/browser/ui/ash/launcher/chrome_launcher_controller.h

@@ -89,11 +89,6 @@ class ChromeLauncherController : public ash::mojom::ShelfObserver,
   // browsers shelf item if needed.
   virtual void SetItemStatus(ash::ShelfID id, ash::ShelfItemStatus status) = 0;
 
-  // Updates the delegate associated with id (which should be a shortcut).
-  virtual void SetShelfItemDelegate(
-      ash::ShelfID id,
-      std::unique_ptr<ash::ShelfItemDelegate> item_delegate) = 0;
-
   // Closes or unpins the shelf item.
   virtual void CloseLauncherItem(ash::ShelfID id) = 0;
 
@@ -202,9 +197,6 @@ class ChromeLauncherController : public ash::mojom::ShelfObserver,
   virtual BrowserShortcutLauncherItemController*
   GetBrowserShortcutLauncherItemController() = 0;
 
-  virtual ash::ShelfItemDelegate* GetShelfItemDelegate(
-      const ash::ShelfID id) = 0;
-
   // Check if the shelf visibility (location, visibility) will change with a new
   // user profile or not. However, since the full visibility calculation of the
   // shelf cannot be performed here, this is only a probability used for

chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.h

@@ -67,9 +67,6 @@ class ChromeLauncherControllerImpl
   const ash::ShelfItem* GetItem(ash::ShelfID id) const override;
   void SetItemType(ash::ShelfID id, ash::ShelfItemType type) override;
   void SetItemStatus(ash::ShelfID id, ash::ShelfItemStatus status) override;
-  void SetShelfItemDelegate(
-      ash::ShelfID id,
-      std::unique_ptr<ash::ShelfItemDelegate> item_delegate) override;
   void CloseLauncherItem(ash::ShelfID id) override;
   bool IsPinned(ash::ShelfID id) override;
   void SetV1AppStatus(const std::string& app_id,
@@ -107,7 +104,6 @@ class ChromeLauncherControllerImpl
       content::WebContents* web_contents) const override;
   BrowserShortcutLauncherItemController*
   GetBrowserShortcutLauncherItemController() override;
-  ash::ShelfItemDelegate* GetShelfItemDelegate(const ash::ShelfID id) override;
   bool ShelfBoundsChangesProbablyWithUser(
       ash::WmShelf* shelf,
       const AccountId& account_id) const override;
@@ -161,7 +157,6 @@ class ChromeLauncherControllerImpl
   friend class TestChromeLauncherControllerImpl;
   FRIEND_TEST_ALL_PREFIXES(ChromeLauncherControllerImplTest, AppPanels);
 
-  typedef std::map<ash::ShelfID, ash::ShelfItemDelegate*> IDToItemControllerMap;
   typedef std::map<content::WebContents*, std::string> WebContentsToAppIDMap;
 
   // Remembers / restores list of running applications.
@@ -171,7 +166,7 @@ class ChromeLauncherControllerImpl
   void RestoreUnpinnedRunningApplicationOrder(const std::string& user_id);
 
   // Invoked when the associated browser or app is closed.
-  void LauncherItemClosed(ash::ShelfID id);
+  void RemoveShelfItem(ash::ShelfID id);
 
   // Internal helpers for pinning and unpinning that handle both
   // client-triggered and internal pinning operations.
@@ -237,8 +232,6 @@ class ChromeLauncherControllerImpl
   void ShelfItemRemoved(int index, const ash::ShelfItem& old_item) override;
   void ShelfItemMoved(int start_index, int target_index) override;
   void ShelfItemChanged(int index, const ash::ShelfItem& old_item) override;
-  void OnSetShelfItemDelegate(ash::ShelfID id,
-                              ash::ShelfItemDelegate* item_delegate) override;
 
   // ash::WindowTreeHostManager::Observer:
   void OnDisplayConfigurationChanged() override;
@@ -261,9 +254,6 @@ class ChromeLauncherControllerImpl
 
   ash::ShelfModel* model_;
 
-  // Controller items in this map are owned by |ShelfModel|.
-  IDToItemControllerMap id_to_item_controller_map_;
-
   // Direct access to app_id for a web contents.
   WebContentsToAppIDMap web_contents_to_app_id_;
 

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

@@ -254,7 +254,7 @@ class LauncherPlatformAppBrowserTest
   }
 
   ash::ShelfItemDelegate* GetShelfItemDelegate(ash::ShelfID id) {
-    return controller_->GetShelfItemDelegate(id);
+    return shelf_model()->GetShelfItemDelegate(id);
   }
 
   ChromeLauncherControllerImpl* controller_;
@@ -788,9 +788,7 @@ IN_PROC_BROWSER_TEST_F(LauncherPlatformAppBrowserTest, AppPanel) {
   const ash::ShelfItem& item1 = GetLastLauncherPanelItem();
   EXPECT_EQ(ash::TYPE_APP_PANEL, item1.type);
   EXPECT_EQ(ash::STATUS_RUNNING, item1.status);
-  EXPECT_EQ(nullptr, GetShelfItemDelegate(item1.id));
-  ash::ShelfItemDelegate* item1_delegate =
-      shelf_model()->GetShelfItemDelegate(item1.id);
+  ash::ShelfItemDelegate* item1_delegate = GetShelfItemDelegate(item1.id);
   EXPECT_EQ(ash::TYPE_APP_PANEL,
             panel->GetNativeWindow()->GetProperty(ash::kShelfItemTypeKey));
   // Click the item and confirm that the panel is activated.
@@ -828,9 +826,7 @@ IN_PROC_BROWSER_TEST_F(LauncherPlatformAppBrowserTest, AppPanelClickBehavior) {
   const ash::ShelfItem& item1 = GetLastLauncherPanelItem();
   EXPECT_EQ(ash::TYPE_APP_PANEL, item1.type);
   EXPECT_EQ(ash::STATUS_RUNNING, item1.status);
-  EXPECT_EQ(nullptr, GetShelfItemDelegate(item1.id));
-  ash::ShelfItemDelegate* item1_delegate =
-      shelf_model()->GetShelfItemDelegate(item1.id);
+  ash::ShelfItemDelegate* item1_delegate = GetShelfItemDelegate(item1.id);
   EXPECT_EQ(ash::TYPE_APP_PANEL,
             panel->GetNativeWindow()->GetProperty(ash::kShelfItemTypeKey));
   // Click the item and confirm that the panel is activated.
@@ -910,8 +906,6 @@ IN_PROC_BROWSER_TEST_F(LauncherPlatformAppBrowserTest, SetIcon) {
   ASSERT_TRUE(app_custom_icon_item_delegate);
   EXPECT_TRUE(app_custom_icon_item_delegate->image_set_by_controller());
 
-  // Panels are handled by ShelfWindowWatcher, not ChromeLauncherController.
-  EXPECT_EQ(nullptr, GetShelfItemDelegate(panel_item.id));
   // Ensure icon heights are correct (see test.js in app_icon/ test directory)
   EXPECT_EQ(extension_misc::EXTENSION_ICON_SMALL, app_item.image.height());
   EXPECT_EQ(extension_misc::EXTENSION_ICON_LARGE,
@@ -1546,10 +1540,7 @@ IN_PROC_BROWSER_TEST_F(LauncherPlatformAppBrowserTest, WindowAttentionStatus) {
   EXPECT_FALSE(panel->GetBaseWindow()->IsActive());
   // Confirm that a shelf item was created and is the correct state.
   const ash::ShelfItem& item = GetLastLauncherPanelItem();
-  // Panels are handled by ShelfWindowWatcher, not ChromeLauncherController.
-  EXPECT_EQ(nullptr, GetShelfItemDelegate(item.id));
-  ash::ShelfItemDelegate* shelf_item_delegate =
-      shelf_model()->GetShelfItemDelegate(item.id);
+  ash::ShelfItemDelegate* shelf_item_delegate = GetShelfItemDelegate(item.id);
   EXPECT_NE(nullptr, shelf_item_delegate);
   EXPECT_EQ(ash::TYPE_APP_PANEL, item.type);
   EXPECT_EQ(ash::STATUS_RUNNING, item.status);
@@ -1774,7 +1765,7 @@ IN_PROC_BROWSER_TEST_F(ShelfAppBrowserTest, ActivateAfterSessionRestore) {
 
   // Now request to either activate an existing app or create a new one.
   ash::ShelfItemDelegate* item_delegate =
-      controller_->GetShelfItemDelegate(shortcut_id);
+      model_->GetShelfItemDelegate(shortcut_id);
   SelectItem(item_delegate, ui::ET_KEY_RELEASED);
 
   // Check that we have set focus on the existing application and nothing new

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

@@ -43,12 +43,6 @@ void ChromeLauncherControllerMus::SetItemStatus(ash::ShelfID id,
   NOTIMPLEMENTED();
 }
 
-void ChromeLauncherControllerMus::SetShelfItemDelegate(
-    ash::ShelfID id,
-    std::unique_ptr<ash::ShelfItemDelegate> item_delegate) {
-  NOTIMPLEMENTED();
-}
-
 void ChromeLauncherControllerMus::CloseLauncherItem(ash::ShelfID id) {
   NOTIMPLEMENTED();
 }
@@ -175,12 +169,6 @@ ChromeLauncherControllerMus::GetBrowserShortcutLauncherItemController() {
   return nullptr;
 }
 
-ash::ShelfItemDelegate* ChromeLauncherControllerMus::GetShelfItemDelegate(
-    const ash::ShelfID id) {
-  NOTIMPLEMENTED();
-  return nullptr;
-}
-
 bool ChromeLauncherControllerMus::ShelfBoundsChangesProbablyWithUser(
     ash::WmShelf* shelf,
     const AccountId& account_id) const {

chrome/browser/ui/ash/launcher/chrome_launcher_controller_mus.h

@@ -22,9 +22,6 @@ class ChromeLauncherControllerMus : public ChromeLauncherController {
   const ash::ShelfItem* GetItem(ash::ShelfID id) const override;
   void SetItemType(ash::ShelfID id, ash::ShelfItemType type) override;
   void SetItemStatus(ash::ShelfID id, ash::ShelfItemStatus status) override;
-  void SetShelfItemDelegate(
-      ash::ShelfID id,
-      std::unique_ptr<ash::ShelfItemDelegate> item_delegate) override;
   void CloseLauncherItem(ash::ShelfID id) override;
   bool IsPinned(ash::ShelfID id) override;
   void SetV1AppStatus(const std::string& app_id,
@@ -62,7 +59,6 @@ class ChromeLauncherControllerMus : public ChromeLauncherController {
       content::WebContents* web_contents) const override;
   BrowserShortcutLauncherItemController*
   GetBrowserShortcutLauncherItemController() override;
-  ash::ShelfItemDelegate* GetShelfItemDelegate(const ash::ShelfID id) override;
   bool ShelfBoundsChangesProbablyWithUser(
       ash::WmShelf* shelf,
       const AccountId& account_id) const override;

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

@@ -5,6 +5,7 @@
 #include "chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.h"
 
 #include "ash/common/shelf/shelf_delegate.h"
+#include "ash/common/shelf/shelf_model.h"
 #include "ash/common/wm_window.h"
 #include "ash/shell.h"
 #include "ash/wm/window_properties.h"
@@ -194,7 +195,8 @@ void ExtensionAppWindowLauncherController::RegisterApp(AppWindow* app_window) {
         item_controller->set_image_set_by_controller(true);
       }
     } else {
-      owner()->SetShelfItemDelegate(shelf_id, std::move(controller));
+      ash::ShelfModel* shelf_model = ash::Shell::Get()->shelf_model();
+      shelf_model->SetShelfItemDelegate(shelf_id, std::move(controller));
     }
 
     // We need to change the controller associated with app_shelf_id.