Refactor LauncherItemController and LauncherItemDelegate

ash/launcher/launcher_item_delegate_manager.h

Index: ash/launcher/launcher_item_delegate_manager.h
diff --git a/ash/launcher/launcher_item_delegate_manager.h b/ash/launcher/launcher_item_delegate_manager.h
index f7c52f5cf6400c20c18bc0ec988615ebd408c204..6f9a5b0e51178f0d915ae10b676e91f2970c7548 100644
--- a/ash/launcher/launcher_item_delegate_manager.h
+++ b/ash/launcher/launcher_item_delegate_manager.h
@@ -8,6 +8,7 @@
 #include <map>
 
 #include "ash/ash_export.h"
+#include "ash/launcher/launcher_model_observer.h"
 #include "ash/launcher/launcher_types.h"
 #include "base/compiler_specific.h"
 
@@ -15,32 +16,55 @@ namespace ash {
 class LauncherItemDelegate;
 
 // LauncherItemDelegateManager helps Launcher/LauncherView to choose right
-// LauncherItemDelegate based on LauncherItemType.
-// When new LauncherItemDelegate is created, it must be registered by
-// RegisterLauncherItemDelegate(). If not, Launcher/LauncherView can't get
-// LauncherItem's LauncherItemDelegate.
-// TODO(simon.hong81): This class should own all LauncherItemDelegate.
-class ASH_EXPORT LauncherItemDelegateManager {
+// LauncherItemDelegate based on LauncherID. When LauncherItem is created, its

[sky, 2013/09/11 01:42:57]

How about: LauncherItemDelegateManager manages the set of LauncherItemDelegates
for the launcher. LauncherItemDelegateManager does not create
LauncherItemDelegates, rather it is expected that someone else invokes
RegisterLauncherItemDelegate appropriately. On the other hand
LauncherItemDelegateManager destroys LauncherItemDelegates when the
corresponding item from the model is removed.

[simonhong_, 2013/09/11 09:05:26]

Thank you for suggesting good description!
Done.

+// LauncherItemDelegate should be registered by RegisterLauncherItemDelegate().
+// When LauncherItemDelegate is changed for existing LauncherItem,
+// use UpdateLauncherItemDelegate().
+// When LauncherItem is removed from LauncherModel, its LauncherItemDelegate is
+// unregistered by LauncherItemRemoved(). So we don't need to call
+// UnregisterLauncherItemDelegate() explicitly.
+// LauncherItemDelegateManager takes ownership of all LauncherItemDelegate.
+class ASH_EXPORT LauncherItemDelegateManager
+    : public ash::LauncherModelObserver {
  public:
   LauncherItemDelegateManager();
   virtual ~LauncherItemDelegateManager();
 
-  // Returns LauncherItemDelegate for |item_type|.
-  // This class doesn't own each LauncherItemDelegate for now.
-  LauncherItemDelegate* GetLauncherItemDelegate(
-      ash::LauncherItemType item_type);
+  // Register |item_delegate| for |id|.
+  void RegisterLauncherItemDelegate(ash::LauncherID id,
+                                    ash::LauncherItemDelegate* item_delegate);

[sky, 2013/09/11 01:42:57]

Can this take a scoped_ptr so that ownership is clear?

[simonhong_, 2013/09/11 09:05:26]

Done.

 
-  // Register |item_delegate| for |type|.
-  // For now, This class doesn't own |item_delegate|.
-  // TODO(simon.hong81): Register LauncherItemDelegate with LauncherID.
-  void RegisterLauncherItemDelegate(
-      ash::LauncherItemType type, LauncherItemDelegate* item_delegate);
+  // Update LauncherItemDelegate for |id| with |item_delegate|.
+  void UpdateLauncherItemDelegate(ash::LauncherID id,

[sky, 2013/09/11 01:42:57]

Why do we need this?

[simonhong_, 2013/09/11 09:05:26]

For the PlatformApp LauncherItem, AppShortcutLauncherItemController isn't used
anymore when it is activated. Instead, ShellWindowLauncherItemController is
created for that Launcheritem.
At this time, LauncherItemDelegate is updated with existing LauncherItem.

[sky, 2013/09/12 20:40:13]

Why can't it just call Register again?

[simonhong_, 2013/09/12 21:06:23]

Ah.. yes, I have a mistake.
It is same as Unregister... and Register
I removed this..

+                                  ash::LauncherItemDelegate* item_delegate);
+
+  // Returns LauncherItemDelegate for |item_type|. Always returns non-NULL.
+  LauncherItemDelegate* GetLauncherItemDelegate(ash::LauncherID id);
+
+  // ash::LauncherModelObserver overrides:
+  virtual void LauncherItemAdded(int model_index) OVERRIDE;
+  virtual void LauncherItemRemoved(int index, ash::LauncherID id) OVERRIDE;
+  virtual void LauncherItemMoved(int start_index, int targetindex) OVERRIDE;
+  virtual void LauncherItemChanged(
+      int index,
+      const ash::LauncherItem& old_item) OVERRIDE;
+  virtual void LauncherStatusChanged() OVERRIDE;
+
+  // Clear all exsiting LauncherItemDelegate for test.
+  void UnregisterAllLauncherItemDelegateForTest();
 
  private:
-  typedef std::map<ash::LauncherItemType, LauncherItemDelegate*>
-      LauncherItemTypeToItemDelegateMap;
+  typedef std::map<ash::LauncherID, LauncherItemDelegate*>
+      LauncherIDToItemDelegateMap;
+
+  // Unregister and destroy LauncherItemDelegate for |id|.
+  void UnregisterLauncherItemDelegate(ash::LauncherID id);
+
+  LauncherIDToItemDelegateMap id_to_item_delegate_map_;
 
-  LauncherItemTypeToItemDelegateMap item_type_to_item_delegate_map_;
+  // Store register/unregister count.
+  int register_count_;
+  int unregister_count_;
 
   DISALLOW_COPY_AND_ASSIGN(LauncherItemDelegateManager);
 };