mash: Refactor ChromeLauncherController's ShelfDelegate code.

chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.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 <vector>
 
@@ skipping 332 matching lines @@
     controller = AppShortcutLauncherItemController::Create(app_launch_id, this);
     controller->set_shelf_id(id);
     // Existing controller is destroyed and replaced by registering again.
     iter->second = controller;
     SetShelfItemDelegate(id, controller);
   } else {
     LauncherItemClosed(id);
   }
 }
 
-void ChromeLauncherControllerImpl::Pin(ash::ShelfID id) {
-  DCHECK(HasShelfIDToAppIDMapping(id));
+void ChromeLauncherControllerImpl::UnpinShelfItemInternal(ash::ShelfID id) {

[James Cook, 2017/03/23 16:43:49]

nit: move to match header order (it's short, I don't care about the git blame
history loss)

[msw, 2017/03/23 18:08:10]

Left as-is. The ordering in this file is a mess... I'm not sure where to put it.
(I'd put it at the end, but that's where AppIconLoaderDelegate overrides live)

   const ash::ShelfItem* item = GetItem(id);
-  if (item && item->type == ash::TYPE_APP)
-    SetItemType(id, ash::TYPE_PINNED_APP);
-  else if (!item || item->type != ash::TYPE_PINNED_APP)
-    return;
-
-  SyncPinPosition(id);
-}
-
-void ChromeLauncherControllerImpl::Unpin(ash::ShelfID id) {
-  UnpinAndUpdatePrefs(id, true /* update_prefs */);
-}
-
-void ChromeLauncherControllerImpl::UnpinAndUpdatePrefs(ash::ShelfID id,
-                                                       bool update_prefs) {
   LauncherItemController* controller = GetLauncherItemController(id);
-  CHECK(controller);
-
-  if (update_prefs) {
-    ash::launcher::RemovePinPosition(
-        profile(),
-        ash::AppLaunchId(GetAppIDForShelfID(id), GetLaunchIDForShelfID(id)));
-  }
-
-  const ash::ShelfItem* item = GetItem(id);
   if (item && (item->status != ash::STATUS_CLOSED || controller->locked()))
     UnpinRunningAppInternal(model_->ItemIndexByID(id));
   else
     LauncherItemClosed(id);
 }
 
 bool ChromeLauncherControllerImpl::IsPinned(ash::ShelfID id) {
   const ash::ShelfItem* item = GetItem(id);
   return item && (item->type == ash::TYPE_PINNED_APP ||
                   item->type == ash::TYPE_BROWSER_SHORTCUT);
 }
 
-void ChromeLauncherControllerImpl::TogglePinned(ash::ShelfID id) {
-  if (!HasShelfIDToAppIDMapping(id))
-    return;  // May happen if item closed with menu open.
-
-  if (IsPinned(id))
-    Unpin(id);
-  else
-    Pin(id);
-}
-
 void ChromeLauncherControllerImpl::LockV1AppWithID(const std::string& app_id) {
   ash::ShelfID id = GetShelfIDForAppID(app_id);
   if (id == ash::kInvalidShelfID) {
     CreateAppShortcutLauncherItemWithType(ash::AppLaunchId(app_id),
                                           model_->item_count(), ash::TYPE_APP);
     id = GetShelfIDForAppID(app_id);
   }
   CHECK(id);
   GetLauncherItemController(id)->lock();
 }
@@ skipping 25 matching lines @@
     return;  // May happen if menu closed.
   delegate->Close();
 }
 
 bool ChromeLauncherControllerImpl::IsOpen(ash::ShelfID id) {
   const ash::ShelfItem* item = GetItem(id);
   return item && item->status != ash::STATUS_CLOSED;
 }
 
 bool ChromeLauncherControllerImpl::IsPlatformApp(ash::ShelfID id) {
-  if (!HasShelfIDToAppIDMapping(id))
-    return false;
-
   std::string app_id = GetAppIDForShelfID(id);
   const Extension* extension = GetExtensionForAppID(app_id, profile());
   // An extension can be synced / updated at any time and therefore not be
   // available.
   return extension ? extension->is_platform_app() : false;
 }
 
 void ChromeLauncherControllerImpl::ActivateApp(const std::string& app_id,
                                                ash::ShelfLaunchSource source,
                                                int event_flags) {
@@ skipping 264 matching lines @@
     if (item && item->type == ash::TYPE_BROWSER_SHORTCUT)
       return static_cast<BrowserShortcutLauncherItemController*>(pair.second);
   }
   NOTREACHED()
       << "There should be always be a BrowserShortcutLauncherItemController.";
   return nullptr;
 }
 
 LauncherItemController* ChromeLauncherControllerImpl::GetLauncherItemController(
     const ash::ShelfID id) {
-  if (!HasShelfIDToAppIDMapping(id))
-    return nullptr;
-  return id_to_item_controller_map_[id];
+  IDToItemControllerMap::iterator iter = id_to_item_controller_map_.find(id);
+  return iter == id_to_item_controller_map_.end() ? nullptr : iter->second;
 }
 
 bool ChromeLauncherControllerImpl::ShelfBoundsChangesProbablyWithUser(
     ash::WmShelf* shelf,
     const AccountId& account_id) const {
   Profile* other_profile = multi_user_util::GetProfileFromAccountId(account_id);
   if (!other_profile || other_profile == profile())
     return false;
 
   // Note: The Auto hide state from preferences is not the same as the actual
@@ skipping 82 matching lines @@
     app_service->AddObserverAndStart(this);
 
   PrefServiceSyncableFromProfile(profile())->AddObserver(this);
 }
 
 ///////////////////////////////////////////////////////////////////////////////
 // ash::ShelfDelegate:
 
 ash::ShelfID ChromeLauncherControllerImpl::GetShelfIDForAppID(
     const std::string& app_id) {
-  // Get shelf id for app_id and empty launch_id.
+  // Get shelf id for |app_id| and an empty |launch_id|.
   return GetShelfIDForAppIDAndLaunchID(app_id, std::string());
 }
 
 ash::ShelfID ChromeLauncherControllerImpl::GetShelfIDForAppIDAndLaunchID(
     const std::string& app_id,

[James Cook, 2017/03/23 16:43:49]

Not for this CL: WDYT of converting this to use AppLaunchId()?

[msw, 2017/03/23 18:08:10]

I'm very much in favor of that; I thought about doing it in this CL.

     const std::string& launch_id) {
+  // TODO(khmel): Fix this Arc application id mapping. See http://b/31703859
   const std::string shelf_app_id =
       ArcAppWindowLauncherController::GetShelfAppIdFromArcAppId(app_id);

[James Cook, 2017/03/23 16:43:49]

Wow, this is terrible. It implies that app_id is always an ARC id. At a minimum
this function needs to be renamed to something like UpdateShelfAppIdForArc() or
MaybeGetArcAppId(). Not for this CL, tho. I commented on the bug.

[msw, 2017/03/23 18:08:10]

Yeah, this is a bad pattern. Yury said he'll work on it.

-  for (const auto& id_to_item_controller_pair : id_to_item_controller_map_) {
-    if (id_to_item_controller_pair.second->app_id() == shelf_app_id &&
-        id_to_item_controller_pair.second->launch_id() == launch_id) {
-      return id_to_item_controller_pair.first;
+
+  for (const ash::ShelfItem& item : model_->items()) {
+    // Ash's ShelfWindowWatcher handles app panel windows separately.
+    if (item.type != ash::TYPE_APP_PANEL &&
+        item.app_launch_id.app_id() == shelf_app_id &&
+        item.app_launch_id.launch_id() == launch_id) {
+      return item.id;
     }
   }
   return ash::kInvalidShelfID;
 }
 
-bool ChromeLauncherControllerImpl::HasShelfIDToAppIDMapping(
-    ash::ShelfID id) const {
-  return id_to_item_controller_map_.find(id) !=
-         id_to_item_controller_map_.end();
-}
-
 const std::string& ChromeLauncherControllerImpl::GetAppIDForShelfID(
     ash::ShelfID id) {
-  LauncherItemController* controller = GetLauncherItemController(id);
-  if (controller)
-    return controller->app_id();
   ash::ShelfItems::const_iterator item = model_->ItemByID(id);
   return item != model_->items().end() ? item->app_launch_id.app_id()
                                        : base::EmptyString();
 }
 
 void ChromeLauncherControllerImpl::PinAppWithID(const std::string& app_id) {
+  // TODO(khmel): Fix this Arc application id mapping. See http://b/31703859
   const std::string shelf_app_id =
       ArcAppWindowLauncherController::GetShelfAppIdFromArcAppId(app_id);
-  if (GetPinnableForAppID(shelf_app_id, profile()) ==
-      AppListControllerDelegate::PIN_EDITABLE)
-    DoPinAppWithID(shelf_app_id);
-  else
-    NOTREACHED();
+
+  // Requests to pin should only be be made for apps with editable pin states.
+  DCHECK_EQ(GetPinnableForAppID(shelf_app_id, profile()),
+            AppListControllerDelegate::PIN_EDITABLE);
+
+  // If the app is already pinned, do nothing and return.
+  if (IsAppPinned(shelf_app_id))
+    return;
+
+  // If there is an existing item, convert that to a pinned item.
+  ash::ShelfID shelf_id = GetShelfIDForAppID(shelf_app_id);
+  if (shelf_id != ash::kInvalidShelfID) {
+    DCHECK_EQ(GetItem(shelf_id)->type, ash::TYPE_APP);
+    DCHECK(!GetItem(shelf_id)->pinned_by_policy);
+    SetItemType(shelf_id, ash::TYPE_PINNED_APP);

[James Cook, 2017/03/23 16:43:50]

Do you need to trigger sync after this?

[msw, 2017/03/23 18:08:10]

Good catch! I missed that! Fixed (but maybe I ruined the function's beauty)

+    return;
+  }
+
+  // Create a new pinned item for the app.
+  shelf_id = CreateAppShortcutLauncherItem(ash::AppLaunchId(shelf_app_id),
+                                           model_->item_count());
+
+  // TODO(msw): Trigger pref updates in ShelfModelObserver overrides.
+  SyncPinPosition(shelf_id);
 }

[James Cook, 2017/03/23 16:43:49]

beautiful function, btw. I like the blocks with single-line comments on each.

[msw, 2017/03/23 18:08:10]

Acknowledged.

 
 bool ChromeLauncherControllerImpl::IsAppPinned(const std::string& app_id) {
+  // TODO(khmel): Fix this Arc application id mapping. See http://b/31703859
   const std::string shelf_app_id =
       ArcAppWindowLauncherController::GetShelfAppIdFromArcAppId(app_id);
-  for (const auto& pair : id_to_item_controller_map_) {
-    if (IsPinned(pair.first) && pair.second->app_id() == shelf_app_id)
-      return true;
-  }
-  return false;
+
+  return IsPinned(GetShelfIDForAppID(shelf_app_id));
 }
 
 void ChromeLauncherControllerImpl::UnpinAppWithID(const std::string& app_id) {
+  // TODO(khmel): Fix this Arc application id mapping. See http://b/31703859
   const std::string shelf_app_id =
       ArcAppWindowLauncherController::GetShelfAppIdFromArcAppId(app_id);
-  if (GetPinnableForAppID(shelf_app_id, profile()) ==
-      AppListControllerDelegate::PIN_EDITABLE)
-    DoUnpinAppWithID(shelf_app_id, true /* update_prefs */);
-  else
-    NOTREACHED();
+
+  // Requests to unpin should only be be made for apps with editable pin states.
+  DCHECK_EQ(GetPinnableForAppID(shelf_app_id, profile()),
+            AppListControllerDelegate::PIN_EDITABLE);
+
+  // If the app is already not pinned, do nothing and return.
+  if (!IsAppPinned(shelf_app_id))
+    return;
+
+  // TODO(msw): Trigger pref updates in ShelfModelObserver overrides.
+  ash::launcher::RemovePinPosition(profile(), ash::AppLaunchId(shelf_app_id));
+
+  // Unpin the shelf item.
+  UnpinShelfItemInternal(GetShelfIDForAppID(shelf_app_id));
 }
 
 ///////////////////////////////////////////////////////////////////////////////
 // LauncherAppUpdater::Delegate:
 
 void ChromeLauncherControllerImpl::OnAppInstalled(
     content::BrowserContext* browser_context,
     const std::string& app_id) {
   if (IsAppPinned(app_id)) {
     // Clear and re-fetch to ensure icon is up-to-date.
@@ skipping 14 matching lines @@
   if (app_icon_loader)
     app_icon_loader->UpdateImage(app_id);
 }
 
 void ChromeLauncherControllerImpl::OnAppUninstalledPrepared(
     content::BrowserContext* browser_context,
     const std::string& app_id) {
   // Since we might have windowed apps of this type which might have
   // outstanding locks which needs to be removed.
   const Profile* profile = Profile::FromBrowserContext(browser_context);
-  if (GetShelfIDForAppID(app_id))
+  ash::ShelfID shelf_id = GetShelfIDForAppID(app_id);
+  if (shelf_id != ash::kInvalidShelfID)
     CloseWindowedAppsFromRemovedExtension(app_id, profile);
 
   if (IsAppPinned(app_id)) {
-    if (profile == this->profile()) {
-      // Some apps may be removed locally. Don't remove pin position from sync
-      // model. When needed, it is automatically deleted on app list model
-      // update.
-      DoUnpinAppWithID(app_id, false /* update_prefs */);
+    if (profile == this->profile() && shelf_id != ash::kInvalidShelfID) {
+      // Some apps may be removed locally. Unpin the item without removing the
+      // pin position from profile preferences. When needed, it is automatically
+      // deleted on app list model update.
+      UnpinShelfItemInternal(shelf_id);
     }
     AppIconLoader* app_icon_loader = GetAppIconLoaderForApp(app_id);
     if (app_icon_loader)
       app_icon_loader->ClearImage(app_id);
   }
 }
 
 ///////////////////////////////////////////////////////////////////////////////
 // ChromeLauncherControllerImpl protected:
 
@@ skipping 71 matching lines @@
   if (app_icon_loader)
     app_icon_loader->ClearImage(app_id);
   id_to_item_controller_map_.erase(iter);
   int index = model_->ItemIndexByID(id);
   // A "browser proxy" is not known to the model and this removal does
   // therefore not need to be propagated to the model.
   if (index != -1)
     model_->RemoveItemAt(index);
 }
 
-void ChromeLauncherControllerImpl::DoPinAppWithID(const std::string& app_id) {
-  // If there is an item, do nothing and return.
-  if (IsAppPinned(app_id))
-    return;
-
-  ash::ShelfID shelf_id = GetShelfIDForAppID(app_id);
-  if (shelf_id) {
-    // App item exists, pin it
-    Pin(shelf_id);
-  } else {
-    // Otherwise, create a shortcut item for it.
-    shelf_id = CreateAppShortcutLauncherItem(ash::AppLaunchId(app_id),
-                                             model_->item_count());
-    SyncPinPosition(shelf_id);
-  }
-}
-
-void ChromeLauncherControllerImpl::DoUnpinAppWithID(const std::string& app_id,
-                                                    bool update_prefs) {
-  ash::ShelfID shelf_id = GetShelfIDForAppID(app_id);
-  if (shelf_id && IsPinned(shelf_id))
-    UnpinAndUpdatePrefs(shelf_id, update_prefs);
-}
-
 void ChromeLauncherControllerImpl::PinRunningAppInternal(
     int index,
     ash::ShelfID shelf_id) {
   DCHECK_EQ(GetItem(shelf_id)->type, ash::TYPE_APP);
   SetItemType(shelf_id, ash::TYPE_PINNED_APP);
   int running_index = model_->ItemIndexByID(shelf_id);
   if (running_index < index)
     --index;
   if (running_index != index)
     model_->Move(running_index, index);
@@ skipping 62 matching lines @@
 
 void ChromeLauncherControllerImpl::ScheduleUpdateAppLaunchersFromPref() {
   base::ThreadTaskRunnerHandle::Get()->PostTask(
       FROM_HERE,
       base::Bind(&ChromeLauncherControllerImpl::UpdateAppLaunchersFromPref,
                  weak_ptr_factory_.GetWeakPtr()));
 }
 
 void ChromeLauncherControllerImpl::UpdateAppLaunchersFromPref() {
   // There are various functions which will trigger a |SyncPinPosition| call
-  // like a direct call to |DoPinAppWithID|, or an indirect call to the menu
+  // like a direct call to |PinAppWithID|, or an indirect call to the menu
   // model which will use weights to re-arrange the icons to new positions.
   // Since this function is meant to synchronize the "is state" with the
   // "sync state", it makes no sense to store any changes by this function back
   // into the pref state. Therefore we tell |persistPinnedState| to ignore any
   // invocations while we are running.
   base::AutoReset<bool> auto_reset(&ignore_persist_pinned_state_change_, true);
   const std::vector<ash::AppLaunchId> pinned_apps =
       ash::launcher::GetPinnedAppsFromPrefs(profile()->GetPrefs(),
                                             launcher_controller_helper());
 
@@ skipping 44 matching lines @@
       // This is fresh pin. Create new one.
       DCHECK_NE(app_id, extension_misc::kChromeAppId);
       CreateAppShortcutLauncherItem(pref_app_launch_id, index);
     }
     ++index;
   }
 
   // At second step remove any pin to the right from the current index.
   while (index < model_->item_count()) {
     const ash::ShelfItem item = model_->items()[index];
-    if (item.type != ash::TYPE_PINNED_APP) {
+    if (item.type == ash::TYPE_PINNED_APP)
+      UnpinShelfItemInternal(item.id);
+    else
       ++index;
-      continue;
-    }
-
-    const LauncherItemController* controller =
-        GetLauncherItemController(item.id);
-    DCHECK(controller);
-    DCHECK_NE(controller->app_id(), extension_misc::kChromeAppId);
-
-    if (item.status != ash::STATUS_CLOSED || controller->locked()) {
-      UnpinRunningAppInternal(index);
-      // Note, item can be moved to the right due weighting in shelf model.
-      DCHECK_GE(model_->ItemIndexByID(item.id), index);
-    } else {
-      LauncherItemClosed(item.id);
-    }
   }
 
   UpdatePolicyPinnedAppsFromPrefs();
 }
 
 void ChromeLauncherControllerImpl::UpdatePolicyPinnedAppsFromPrefs() {
   for (int index = 0; index < model_->item_count(); index++) {
     ash::ShelfItem item = model_->items()[index];
     const bool pinned_by_policy =
         GetPinnableForAppID(item.app_launch_id.app_id(), profile()) ==
@@ skipping 47 matching lines @@
   }
   return status;
 }
 
 ash::ShelfID ChromeLauncherControllerImpl::InsertAppLauncherItem(
     LauncherItemController* controller,
     ash::ShelfItemStatus status,
     int index,
     ash::ShelfItemType shelf_item_type) {
   ash::ShelfID id = model_->next_id();
-  CHECK(!HasShelfIDToAppIDMapping(id));
+  CHECK(!GetItem(id));
   CHECK(controller);
   // Ash's ShelfWindowWatcher handles app panel windows separately.
   DCHECK_NE(ash::TYPE_APP_PANEL, shelf_item_type);
   id_to_item_controller_map_[id] = controller;
   controller->set_shelf_id(id);
 
   ash::ShelfItem item;
   item.type = shelf_item_type;
   item.app_launch_id = controller->app_launch_id();
   item.image = extensions::util::GetDefaultAppIcon();
@@ skipping 17 matching lines @@
   SetShelfItemDelegate(id, controller);
   return id;
 }
 
 void ChromeLauncherControllerImpl::CreateBrowserShortcutLauncherItem() {
   ash::ShelfItem browser_shortcut;
   browser_shortcut.type = ash::TYPE_BROWSER_SHORTCUT;
   ResourceBundle& rb = ResourceBundle::GetSharedInstance();
   browser_shortcut.image = *rb.GetImageSkiaNamed(IDR_PRODUCT_LOGO_32);
   browser_shortcut.title = l10n_util::GetStringUTF16(IDS_PRODUCT_NAME);
+  browser_shortcut.app_launch_id =
+      ash::AppLaunchId(extension_misc::kChromeAppId);
   ash::ShelfID id = model_->next_id();
   model_->AddAt(0, browser_shortcut);
   BrowserShortcutLauncherItemController* controller =
       new BrowserShortcutLauncherItemController(this, model_);
   controller->set_shelf_id(id);
   id_to_item_controller_map_[id] = controller;
   // ShelfModel owns BrowserShortcutLauncherItemController.
   SetShelfItemDelegate(id, controller);
   controller->UpdateBrowserItemState();
 }
@@ skipping 87 matching lines @@
   LOG(ERROR) << "Unexpected removal of shelf item, id: " << id;
   id_to_item_controller_map_.erase(iter);
 }
 
 void ChromeLauncherControllerImpl::ShelfItemMoved(int start_index,
                                                   int target_index) {
   const ash::ShelfItem& item = model_->items()[target_index];
   // We remember the moved item position if it is either pinnable or
   // it is the app list with the alternate shelf layout.
   DCHECK_NE(ash::TYPE_APP_LIST, item.type);
-  if (HasShelfIDToAppIDMapping(item.id) && IsPinned(item.id))
+  if (IsPinned(item.id))
     SyncPinPosition(item.id);
 }
 
 void ChromeLauncherControllerImpl::ShelfItemChanged(
     int index,
     const ash::ShelfItem& old_item) {}
 
 void ChromeLauncherControllerImpl::OnSetShelfItemDelegate(
     ash::ShelfID id,
     ash::mojom::ShelfItemDelegate* item_delegate) {
@@ skipping 52 matching lines @@
     LauncherItemController* controller = GetLauncherItemController(item.id);
     if (!controller || controller->image_set_by_controller())
       continue;
     item.image = image;
     if (arc_deferred_launcher_)
       arc_deferred_launcher_->MaybeApplySpinningEffect(id, &item.image);
     model_->Set(index, item);
     // It's possible we're waiting on more than one item, so don't break.
   }
 }