mash: Update shelf pin prefs in ShelfModelObserver overrides.

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

Index: chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc
diff --git a/chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc b/chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc
index 05512255f535988732c8fc65d370b96a9288baf6..5f7a6d1b292133a8e31a4559669c7023ab240580 100644
--- a/chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc
+++ b/chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc
@@ -112,6 +112,12 @@ void SelectItemWithSource(ash::mojom::ShelfItemDelegate* delegate,
                          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.
@@ -361,8 +367,7 @@ void ChromeLauncherControllerImpl::UnpinShelfItemInternal(ash::ShelfID 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) {
@@ -850,9 +855,6 @@ void ChromeLauncherControllerImpl::PinAppWithID(const std::string& app_id) {
     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) {
@@ -872,15 +874,9 @@ void ChromeLauncherControllerImpl::UnpinAppWithID(const std::string& app_id) {
   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));
 }
 
 ///////////////////////////////////////////////////////////////////////////////
@@ -1036,9 +1032,8 @@ void ChromeLauncherControllerImpl::UnpinRunningAppInternal(int index) {
 }
 
 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);
@@ -1097,14 +1092,11 @@ void ChromeLauncherControllerImpl::ScheduleUpdateAppLaunchersFromPref() {
 }
 
 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());
@@ -1267,6 +1259,11 @@ ash::ShelfID ChromeLauncherControllerImpl::InsertAppLauncherItem(
 }
 
 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();
@@ -1361,35 +1358,63 @@ void ChromeLauncherControllerImpl::ReleaseProfile() {
 ///////////////////////////////////////////////////////////////////////////////
 // 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,