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 60 matching lines @@
 #include "chrome/grit/generated_resources.h"
 #include "chrome/grit/theme_resources.h"
 #include "components/favicon/content/content_favicon_driver.h"
 #include "components/prefs/scoped_user_pref_update.h"
 #include "components/signin/core/account_id/account_id.h"
 #include "components/strings/grit/components_strings.h"
 #include "components/sync_preferences/pref_service_syncable.h"
 #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"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/base/window_open_disposition.h"
 #include "ui/display/types/display_constants.h"
 #include "ui/keyboard/keyboard_util.h"
 #include "ui/resources/grit/ui_resources.h"
 #include "ui/wm/core/window_animations.h"
 
@@ skipping 13 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 31 matching lines @@
       // 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);
   }
 }
 
+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:
 
 void ChromeLauncherControllerImpl::OnInit() {
   CreateBrowserShortcutLauncherItem();
   UpdateAppLaunchersFromPref();
 
   // TODO(sky): update unit test so that this test isn't necessary.
   if (ash::Shell::HasInstance())
     SetVirtualKeyboardBehaviorFromPrefs();
@@ skipping 315 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;

[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();
   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) && !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,
     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.
   }
 }