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..41eed871a63ff9eff675b4f13d0caed59cd3b2e4 100644
--- a/chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc
+++ b/chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc
@@ -78,7 +78,9 @@
 #include "components/user_manager/user_manager.h"
 #include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/web_contents.h"
+#include "extensions/browser/extension_system.h"

[James Cook, 2017/03/29 14:28:02]

Are these new includes needed?

[msw, 2017/03/29 21:34:17]

Nope, thanks for catching that; removed.

 #include "extensions/common/extension.h"
+#include "extensions/common/one_shot_event.h"
 #include "ui/aura/window.h"
 #include "ui/aura/window_event_dispatcher.h"
 #include "ui/base/l10n/l10n_util.h"
@@ -112,6 +114,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 +369,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 +857,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 +876,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));
 }
 
 ///////////////////////////////////////////////////////////////////////////////
@@ -932,6 +930,15 @@ void ChromeLauncherControllerImpl::OnAppUninstalledPrepared(
   }
 }
 
+void ChromeLauncherControllerImpl::OnAppDisabling(const std::string& app_id) {
+  // Avoid removing pin positions of ARC apps when ARC itself is disabled.

[msw, 2017/03/29 01:29:56]

This is unfortunate and I'm receptive to better approaches. We generally want
CLC to remove items from the sync model when they are removed from the shelf
model, but we can't do that when ARC is being disabled (and ARC apps are being
removed from the ShelfModel), because we want those apps to remain in the sync
model for other devices (and to restore if ARC is re-enabled).

+  keep_pin_positions_.insert(app_id);
+}
+
+void ChromeLauncherControllerImpl::OnAppDisabled(const std::string& app_id) {
+  keep_pin_positions_.erase(app_id);
+}
+
 ///////////////////////////////////////////////////////////////////////////////
 // ChromeLauncherControllerImpl protected:
 
@@ -1267,6 +1274,11 @@ ash::ShelfID ChromeLauncherControllerImpl::InsertAppLauncherItem(
 }
 
 void ChromeLauncherControllerImpl::CreateBrowserShortcutLauncherItem() {
+  // Do not sync the pin position of the browser shortcut item when it is added;

[msw, 2017/03/29 01:29:56]

This threw me for a loop... I tried other approaches in earlier patch sets
(delaying SyncPinPosition if AppListSyncableService is not created/initialized),
but everything I tried caused weird test failures (adding extensions later on
would not update the sync model, etc.), even though that delay mechanism was not
even triggered in that case! I seriously spent too long on this already, but if
anyone has a better solution, I'm willing to try something else.

+  // its initial position before prefs have loaded is unimportant and the sync
+  // service may not yet be initialized.
+  base::AutoReset<bool> auto_reset(&ignore_persist_pinned_state_change_, true);
+
   ash::ShelfItem browser_shortcut;
   browser_shortcut.type = ash::TYPE_BROWSER_SHORTCUT;
   ResourceBundle& rb = ResourceBundle::GetSharedInstance();
@@ -1361,35 +1373,66 @@ 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) && !ignore_persist_pinned_state_change_)

[James Cook, 2017/03/29 14:28:02]

SyncPinPosition() checks ignore_persist_pinned_state_change_. Do you need to do
it in both places?

It might be clearer to always do it outside the call, since RemovePinPosition
can't check it. Or make a version of RemovePinPosition inside this class that
checks.

[msw, 2017/03/29 21:34:17]

I changed the check inside SyncPinPosition to a DCHECK. I'd like to avoid adding
a RemovePinPosition helper for now.

+    SyncPinPosition(item.id);
+}
 
 void ChromeLauncherControllerImpl::ShelfItemRemoved(
     int index,
     const ash::ShelfItem& old_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());
+
+  // Remove the pin position from preferences as needed. Do not remove positions
+  // of ARC items removed due to ARC being disabled, see |keep_pin_positions_|.
+  if (ItemTypeIsPinned(old_item) && !ignore_persist_pinned_state_change_ &&
+      keep_pin_positions_.count(shelf_app_id) == 0) {
+    ash::AppLaunchId app_launch_id(shelf_app_id,
+                                   old_item.app_launch_id.launch_id());

[James Cook, 2017/03/29 14:28:02]

Is it valid to recycle the launch_id when GetShelfAppIdFromArcAppId() might have
changed the app_id?

[msw, 2017/03/29 21:34:17]

Yes, this translation is only done for arc::kPlayStoreAppId /
ArcSupportHost::kHostAppId, which is only pinned with an empty 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;
-  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) && !ignore_persist_pinned_state_change_)
     SyncPinPosition(item.id);
 }
 
 void ChromeLauncherControllerImpl::ShelfItemChanged(
     int index,
-    const ash::ShelfItem& old_item) {}
+    const ash::ShelfItem& old_item) {
+  if (ignore_persist_pinned_state_change_)
+    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,