mash: Update shelf pin prefs in ShelfModelObserver overrides.

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 94 matching lines @@
 // A callback that does nothing after shelf item selection handling.
 void NoopCallback(ash::ShelfAction, base::Optional<MenuItemList>) {}
 
 // Calls ItemSelected with |source|, default arguments, and no callback.
 void SelectItemWithSource(ash::mojom::ShelfItemDelegate* delegate,
                           ash::ShelfLaunchSource source) {
   delegate->ItemSelected(nullptr, display::kInvalidDisplayId, source,
                          base::Bind(&NoopCallback));
 }
 
+// Returns true if the given |item| has a pinned shelf item type.
+bool ItemTypeIsPinned(const ash::ShelfItem& item) {
+  return item.type == ash::TYPE_PINNED_APP ||
+         item.type == ash::TYPE_BROWSER_SHORTCUT;
+}
+
 }  // namespace
 
 // A class to get events from ChromeOS when a user gets changed or added.
 class ChromeLauncherControllerUserSwitchObserver
     : public user_manager::UserManager::UserSessionStateObserver {
  public:
   ChromeLauncherControllerUserSwitchObserver(
       ChromeLauncherControllerImpl* controller)
       : controller_(controller) {
     DCHECK(user_manager::UserManager::IsInitialized());
@@ skipping 229 matching lines @@
   const ash::ShelfItem* item = GetItem(id);
   LauncherItemController* controller = GetLauncherItemController(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);
+  return item && ItemTypeIsPinned(*item);
 }
 
 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);
@@ skipping 467 matching lines @@
   // Convert an existing item to be pinned, or create a new 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);
   } else {
     shelf_id = CreateAppShortcutLauncherItem(ash::AppLaunchId(shelf_app_id),
                                              model_->item_count());
   }
-
-  // TODO(msw): Trigger pref updates in ShelfModelObserver overrides.
-  SyncPinPosition(shelf_id);
 }
 
 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);
 
   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);
 
   // 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));
+  // If the app is pinned, unpin the shelf item (and remove it if not running).
+  if (IsAppPinned(shelf_app_id))
+    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 135 matching lines @@
 }
 
 void ChromeLauncherControllerImpl::UnpinRunningAppInternal(int index) {
   DCHECK(index >= 0 && index < model_->item_count());
   ash::ShelfItem item = model_->items()[index];
   DCHECK_EQ(item.type, ash::TYPE_PINNED_APP);
   SetItemType(item.id, ash::TYPE_APP);
 }
 
 void ChromeLauncherControllerImpl::SyncPinPosition(ash::ShelfID shelf_id) {
+  DCHECK(should_sync_pin_changes());
   DCHECK(shelf_id);
-  if (ignore_persist_pinned_state_change_)
-    return;
 
   const int max_index = model_->item_count();
   const int index = model_->ItemIndexByID(shelf_id);
   DCHECK_GT(index, 0);
 
   const std::string& app_id = GetAppIDForShelfID(shelf_id);
   DCHECK(!app_id.empty());
   const std::string& launch_id = GetLaunchIDForShelfID(shelf_id);
 
   std::string app_id_before;
@@ skipping 38 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 |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);
+  // Do not sync pin changes during this function to avoid cyclical updates.
+  // This function makes the shelf model reflect synced prefs, and should not
+  // cyclically trigger sync changes (eg. ShelfItemAdded calls SyncPinPosition).
+  auto do_not_sync_pin_changes = DoNotSyncPinChanges();

[James Cook, 2017/03/29 23:23:56]

nit: don't use auto, here or below. "auto" masks the fact that this is a scoped
lock.

[msw, 2017/03/30 00:33:29]

Done.

+
   const std::vector<ash::AppLaunchId> pinned_apps =
       ash::launcher::GetPinnedAppsFromPrefs(profile()->GetPrefs(),
                                             launcher_controller_helper());
 
   int index = 0;
   // Skip app list items if it exists.
   if (model_->items()[0].type == ash::TYPE_APP_LIST)
     ++index;
 
   // Apply pins in two steps. At the first step, go through the list of apps to
@@ skipping 142 matching lines @@
   if (app_icon_loader) {
     app_icon_loader->FetchImage(app_id);
     app_icon_loader->UpdateImage(app_id);
   }
 
   SetShelfItemDelegate(id, controller);
   return id;
 }
 
 void ChromeLauncherControllerImpl::CreateBrowserShortcutLauncherItem() {
+  // Do not sync the pin position of the browser shortcut item when it is added;
+  // its initial position before prefs have loaded is unimportant and the sync
+  // service may not yet be initialized.
+  auto do_not_sync_pin_changes = DoNotSyncPinChanges();
+
   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 =
@@ skipping 74 matching lines @@
       app_list::AppListSyncableServiceFactory::GetForProfile(profile());
   if (app_service)
     app_service->RemoveObserver(this);
 
   PrefServiceSyncableFromProfile(profile())->RemoveObserver(this);
 }
 
 ///////////////////////////////////////////////////////////////////////////////
 // ash::ShelfModelObserver:
 
-void ChromeLauncherControllerImpl::ShelfItemAdded(int index) {}
+void ChromeLauncherControllerImpl::ShelfItemAdded(int index) {
+  // Update the pin position preference as needed.
+  const ash::ShelfItem& item = model_->items()[index];
+  if (ItemTypeIsPinned(item) && should_sync_pin_changes())
+    SyncPinPosition(item.id);
+}
 
 void ChromeLauncherControllerImpl::ShelfItemRemoved(
     int index,
     const ash::ShelfItem& old_item) {
+  // Remove the pin position from preferences as needed.
+  if (ItemTypeIsPinned(old_item) && should_sync_pin_changes()) {
+    // TODO(khmel): Fix this Arc application id mapping. See http://b/31703859
+    const std::string shelf_app_id =
+        ArcAppWindowLauncherController::GetShelfAppIdFromArcAppId(
+            old_item.app_launch_id.app_id());
+    ash::AppLaunchId app_launch_id(shelf_app_id,
+                                   old_item.app_launch_id.launch_id());
+    ash::launcher::RemovePinPosition(profile(), app_launch_id);
+  }
+
   // TODO(skuhne): This fixes crbug.com/429870, but it does not answer why we
   // get into this state in the first place.
-  IDToItemControllerMap::iterator iter =
-      id_to_item_controller_map_.find(old_item.id);
-  if (iter == id_to_item_controller_map_.end())
-    return;
-
-  LOG(ERROR) << "Unexpected removal of shelf item, id: " << old_item.id;

[khmel, 2017/03/29 21:54:10]

nit: may be leave error logging?

[msw, 2017/03/29 22:26:06]

This is not really unexpected anymore; shelf items may be removed by the shelf
components in Ash directly from here on out. We should also remove the map...

-  id_to_item_controller_map_.erase(iter);
+  if (id_to_item_controller_map_.count(old_item.id) > 0)
+    id_to_item_controller_map_.erase(old_item.id);
 }
 
 void ChromeLauncherControllerImpl::ShelfItemMoved(int start_index,
                                                   int target_index) {
+  // Update the pin position preference as needed.
   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 (IsPinned(item.id))
+  if (ItemTypeIsPinned(item) && should_sync_pin_changes())
     SyncPinPosition(item.id);
 }
 
 void ChromeLauncherControllerImpl::ShelfItemChanged(
     int index,
-    const ash::ShelfItem& old_item) {}
+    const ash::ShelfItem& old_item) {
+  if (!should_sync_pin_changes())
+    return;
+
+  const ash::ShelfItem& item = model_->items()[index];
+  // Add or remove the pin position from preferences as needed.
+  if (!ItemTypeIsPinned(old_item) && ItemTypeIsPinned(item)) {
+    SyncPinPosition(item.id);
+  } else if (ItemTypeIsPinned(old_item) && !ItemTypeIsPinned(item)) {
+    // TODO(khmel): Fix this Arc application id mapping. See http://b/31703859
+    const std::string shelf_app_id =
+        ArcAppWindowLauncherController::GetShelfAppIdFromArcAppId(
+            old_item.app_launch_id.app_id());
+
+    ash::AppLaunchId app_launch_id(shelf_app_id,
+                                   old_item.app_launch_id.launch_id());
+    ash::launcher::RemovePinPosition(profile(), app_launch_id);
+  }
+}
 
 void ChromeLauncherControllerImpl::OnSetShelfItemDelegate(
     ash::ShelfID id,
     ash::mojom::ShelfItemDelegate* item_delegate) {
   // TODO(skuhne): This fixes crbug.com/429870, but it does not answer why we
   // get into this state in the first place.
   IDToItemControllerMap::iterator iter = id_to_item_controller_map_.find(id);
   if (iter == id_to_item_controller_map_.end() || item_delegate == iter->second)
     return;
   LOG(ERROR) << "Unexpected change of shelf item delegate, id: " << id;
@@ skipping 46 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.
   }
 }