Refactor LauncherDelegate

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

Index: chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_app.cc
diff --git a/chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_app.cc b/chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_app.cc
index 71b0881c91087e7c0ee328d0a958dbca58858a4e..efe1c5c1021b52b9eb25f9819a2c8aa71caee6f2 100644
--- a/chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_app.cc
+++ b/chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_app.cc
@@ -700,12 +700,6 @@ void ChromeLauncherControllerPerApp::CreateNewIncognitoWindow() {
                          chrome::HOST_DESKTOP_TYPE_ASH);
 }
 
-bool ChromeLauncherControllerPerApp::CanPin() const {
-  const PrefService::Preference* pref =
-      profile_->GetPrefs()->FindPreference(prefs::kPinnedLauncherApps);
-  return pref && pref->IsUserModifiable();
-}
-
 void ChromeLauncherControllerPerApp::PersistPinnedState() {
   if (ignore_persist_pinned_state_change_)
     return;
@@ -945,6 +939,20 @@ ash::LauncherMenuModel* ChromeLauncherControllerPerApp::CreateApplicationMenu(
                                                                  event_flags));
 }
 
+bool ChromeLauncherControllerPerApp::IsDraggable(
+    const ash::LauncherItem& item) {
+  return (item.type == ash::TYPE_APP_SHORTCUT ||
+          item.type == ash::TYPE_WINDOWED_APP) ? CanPin() : true;
+}
+
+bool ChromeLauncherControllerPerApp::ShouldShowTooltip(
+    const ash::LauncherItem& item) {
+  if (item.type == ash::TYPE_APP_PANEL &&
+      id_to_item_controller_map_[item.id]->IsVisible())
+    return false;
+  return true;
+}
+
 ash::LauncherID ChromeLauncherControllerPerApp::GetIDByWindow(
     aura::Window* window) {
   for (IDToItemControllerMap::const_iterator i =
@@ -972,20 +980,6 @@ ash::LauncherID ChromeLauncherControllerPerApp::GetIDByWindow(
   return 0;
 }
 
-bool ChromeLauncherControllerPerApp::IsDraggable(
-    const ash::LauncherItem& item) {
-  return (item.type == ash::TYPE_APP_SHORTCUT ||
-          item.type == ash::TYPE_WINDOWED_APP) ? CanPin() : true;
-}
-
-bool ChromeLauncherControllerPerApp::ShouldShowTooltip(
-    const ash::LauncherItem& item) {
-  if (item.type == ash::TYPE_APP_PANEL &&
-      id_to_item_controller_map_[item.id]->IsVisible())
-    return false;
-  return true;
-}
-
 void ChromeLauncherControllerPerApp::OnLauncherCreated(
     ash::Launcher* launcher) {
   launchers_.insert(launcher);
@@ -999,6 +993,23 @@ void ChromeLauncherControllerPerApp::OnLauncherDestroyed(
   // Launcher is already in its destructor.
 }
 
+// TODO: Remove LauncherItemDelegate interface from
+// ChromeLauncherControllerPerApp. LauncherItemDelegate should be derived by
+// LauncherItemController after removing PerBrowserLauncher.
+ash::LauncherItemDelegate*
+ChromeLauncherControllerPerApp::GetLauncherItemDelegate(
+    const ash::LauncherItem& item) {
+  IDToItemControllerMap::iterator iter =
+      id_to_item_controller_map_.find(item.id);
+  return iter != id_to_item_controller_map_.end() ? this : NULL;
+}
+
+bool ChromeLauncherControllerPerApp::CanPin() const {
+  const PrefService::Preference* pref =
+      profile_->GetPrefs()->FindPreference(prefs::kPinnedLauncherApps);
+  return pref && pref->IsUserModifiable();
+}
+
 void ChromeLauncherControllerPerApp::LauncherItemAdded(int index) {
 }
 
@@ -1019,8 +1030,8 @@ void ChromeLauncherControllerPerApp::LauncherItemChanged(
     int index,
     const ash::LauncherItem& old_item) {
   ash::LauncherID id = model_->items()[index].id;
-  DCHECK(HasItemController(id));
-  id_to_item_controller_map_[id]->LauncherItemChanged(index, old_item);
+  if (HasItemController(id))

[Mr4D (OOO till 08-26), 2013/08/13 18:18:24]

Do you know why this is necessary? We shouldn't come here in the first place if
an item is not valid.

[simonhong_, 2013/08/13 19:54:56]

This is needed because we have two LauncherDelegate.
LauncherModel notifies to its observers regardless of the changing source.
When AshLauncherDelegate change LauncherItem (ex, app list item), this
notification also reaches to here.

[Mr4D (OOO till 08-26), 2013/08/14 15:52:19]

Which 2? Are you talking about the "Ash" and the "Chrome" portion above? If so -
please read my comments above.

[simonhong_, 2013/08/14 23:47:51]

Sorry, I wrote wrong answer.
Yes, currently we can't reach here and also with this CL.
But, think about LauncherItem for dialog (for TaskManager).
When LauncherItem is changed by LauncherModel::Set(), LauncherModel call
LauncherItemChanged() from observer list.
Then, we can reach here.
I think DCHECK should be removed because we will add LauncherItem that not
handled by ChromeLauncherController.

[Mr4D (OOO till 08-26), 2013/08/15 15:53:19]

So we plan that we do not have a ChromeLauncherController for some items? And as
such we will run into many new special cases after you have spent so much time
to make everything nice and clean? Why not?

+    id_to_item_controller_map_[id]->LauncherItemChanged(index, old_item);
 }
 
 void ChromeLauncherControllerPerApp::LauncherStatusChanged() {