Fix the bug that dragging not pinnable app can be pinned.

The function that we usually checked if an app is drag-to-pinnable
didn't check its app type, while the app type is checked when a
context menu is shown and see if a menu item that allows users to
pin/unpin an app should be added. To support this check, this cl adds
a new variable in ShelfItem to mark that the item is able to change
pin state.

Bug: b:276394178, b:265429503
Change-Id: Ie554e9e40c8d80b14f7b918cdb181025922cc7cd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4484719
Commit-Queue: Wen-Chien Wang <wcwang@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1142817}

ash/public/cpp/shelf_item.cc

@@ -10,4 +10,8 @@ ShelfItem::ShelfItem() = default;
 ShelfItem::ShelfItem(const ShelfItem& shelf_item) = default;
 ShelfItem::~ShelfItem() = default;
 
+bool ShelfItem::IsPinStateForced() const {
+  return pinned_by_policy || pin_state_forced_by_type;
+}
+
 }  // namespace ash

ash/public/cpp/shelf_item.h

@@ -20,6 +20,9 @@ struct ASH_PUBLIC_EXPORT ShelfItem {
   ShelfItem(const ShelfItem& shelf_item);
   ~ShelfItem();
 
+  // Returns true if the pin state of the item is forced and can not be changed.
+  bool IsPinStateForced() const;
+
   ShelfItemType type = TYPE_UNDEFINED;
 
   // Image to display in the shelf.
@@ -51,6 +54,10 @@ struct ASH_PUBLIC_EXPORT ShelfItem {
   // not be modifiable by user.
   bool pinned_by_policy = false;
 
+  // Whether the item pin state is forced according to its app type. The pin
+  // state can not be modified by user if this is set to true.
+  bool pin_state_forced_by_type = false;
+
   // Whether the item has a notification.
   bool has_notification = false;
 };

ash/public/cpp/shelf_model.cc

@@ -79,7 +79,7 @@ void ShelfModel::PinExistingItemWithID(const std::string& app_id) {
 
   ShelfItem item = items_[index];
   DCHECK_EQ(item.type, TYPE_APP);
-  DCHECK(!item.pinned_by_policy);
+  DCHECK(!item.IsPinStateForced());
   item.type = TYPE_PINNED_APP;
   Set(index, item);
 }

ash/shelf/shelf_test_util.cc

@@ -40,6 +40,25 @@ ShelfItem ShelfTestUtil::AddAppShortcutWithIcon(const std::string& id,
   return item;
 }
 
+// static
+ShelfItem ShelfTestUtil::AddAppNotPinnable(const std::string& id) {
+  ShelfController* controller = Shell::Get()->shelf_controller();
+  ShelfItem item;
+  item.type = TYPE_APP;
+  item.status = STATUS_RUNNING;
+  item.id = ShelfID(id);
+  item.pin_state_forced_by_type = true;
+
+  // All focusable objects are expected to have an accessible name to pass
+  // the accessibility paint checks. ShelfAppButton will use the item's
+  // title as the accessible name. Since this item is purely for testing,
+  // use its id as the title in order for the unit tests to pass the checks.
+  item.title = base::UTF8ToUTF16(id);
+  controller->model()->Add(item,
+                           std::make_unique<TestShelfItemDelegate>(item.id));
+  return item;
+}
+
 void WaitForOverviewAnimation(bool enter) {
   ShellTestApi().WaitForOverviewAnimationState(
       enter ? OverviewAnimationState::kEnterAnimationComplete

ash/shelf/shelf_test_util.h

@@ -29,6 +29,9 @@ class ShelfTestUtil {
   static ShelfItem AddAppShortcutWithIcon(const std::string& id,
                                           ShelfItemType type,
                                           gfx::ImageSkia icon);
+
+  // Adds an app that is not pinnable to the shelf model.
+  static ShelfItem AddAppNotPinnable(const std::string& id);
 };
 
 // Waits for an overview enter animation if |enter|; waits for an overview exit

ash/shelf/shelf_view.cc

@@ -2000,7 +2000,7 @@ bool ShelfView::CanDragAcrossSeparator(views::View* drag_view) const {
 
   // Note that |drag_and_drop_shelf_id_| is set only when the current drag view
   // is from app list, which can not be dragged to the unpinned app side.
-  return !ShelfItemForView(drag_view)->pinned_by_policy &&
+  return !ShelfItemForView(drag_view)->IsPinStateForced() &&
          drag_and_drop_shelf_id_ == ShelfID() && can_change_pin_state;
 }
 

ash/shelf/shelf_view_unittest.cc

@@ -31,6 +31,7 @@
 #include "ash/shelf/home_button.h"
 #include "ash/shelf/shelf.h"
 #include "ash/shelf/shelf_app_button.h"
+#include "ash/shelf/shelf_controller.h"
 #include "ash/shelf/shelf_focus_cycler.h"
 #include "ash/shelf/shelf_navigation_widget.h"
 #include "ash/shelf/shelf_observer.h"
@@ -868,6 +869,36 @@ TEST_F(ShelfViewTest, DragAppsToPin) {
   EXPECT_EQ(test_api_->GetSeparatorIndex(), absl::nullopt);
 }
 
+// Ensure that the unpinnable apps can not be pinned by dragging.
+TEST_F(ShelfViewTest, NotPinnableItemCantBePinnedByDragging) {
+  std::vector<std::pair<ShelfID, views::View*>> id_map;
+  SetupForDragTest(&id_map);
+  size_t pinned_apps_size = id_map.size();
+
+  // Add an unpinnable app.
+  const ShelfItem unpinnable_app =
+      ShelfTestUtil::AddAppNotPinnable(base::NumberToString(id_++));
+  const ShelfID id = unpinnable_app.id;
+  id_map.emplace_back(id, GetButtonByID(id));
+
+  ASSERT_TRUE(GetButtonByID(id)->state() & ShelfAppButton::STATE_RUNNING);
+  ASSERT_FALSE(IsAppPinned(id));
+  EXPECT_EQ(test_api_->GetSeparatorIndex(), pinned_apps_size - 1);
+
+  // Drag an unpinnable app and move it to the beginning of the shelf. The app
+  // can not be moved across the separator so the dragged app will stay unpinned
+  // beside the separator after release.
+  views::View* dragged_button =
+      SimulateDrag(ShelfView::MOUSE, id_map.size() - 1, 0, false);
+  EXPECT_EQ(1, GetHapticTickEventsCount());
+  ASSERT_NO_FATAL_FAILURE(CheckModelIDs(id_map));
+  shelf_view_->PointerReleasedOnButton(dragged_button, ShelfView::MOUSE, false);
+  test_api_->RunMessageLoopUntilAnimationsDone();
+  EXPECT_EQ(1, GetHapticTickEventsCount());
+  EXPECT_EQ(test_api_->GetSeparatorIndex(), pinned_apps_size - 1);
+  EXPECT_FALSE(IsAppPinned(id));
+}
+
 // Check that separator index updates as expected when a drag view is dragged
 // over it.
 TEST_F(ShelfViewTest, DragAppAroundSeparator) {

chrome/browser/ash/extensions/autotest_private/autotest_private_api.cc

@@ -3947,6 +3947,7 @@ ExtensionFunction::ResponseAction AutotestPrivateGetShelfItemsFunction::Run() {
     result_item.status = GetShelfItemStatus(item.status);
     result_item.shows_tooltip = item.shows_tooltip;
     result_item.pinned_by_policy = item.pinned_by_policy;
+    result_item.pin_state_forced_by_type = item.pin_state_forced_by_type;
     result_item.has_notification = item.has_notification;
     result_items.emplace_back(std::move(result_item));
   }

chrome/browser/ui/ash/shelf/app_service/app_service_shelf_context_menu.cc

@@ -34,6 +34,7 @@
 #include "chrome/browser/ui/ash/shelf/arc_app_shelf_id.h"
 #include "chrome/browser/ui/ash/shelf/browser_shortcut_shelf_item_controller.h"
 #include "chrome/browser/ui/ash/shelf/chrome_shelf_controller.h"
+#include "chrome/browser/ui/ash/shelf/chrome_shelf_controller_util.h"
 #include "chrome/browser/ui/chrome_pages.h"
 #include "chrome/browser/ui/views/crostini/crostini_app_restart_dialog.h"
 #include "chrome/browser/ui/webui/settings/ash/app_management/app_management_uma.h"
@@ -350,8 +351,9 @@ void AppServiceShelfContextMenu::OnGetMenuModel(GetMenuModelCallback callback,
     ++index;
   }
 
-  if (ShouldAddPinMenu())
+  if (IsAppPinEditable(app_type_, item().id.app_id, controller()->profile())) {
     AddPinMenu(menu_model.get());
+  }
 
   size_t shortcut_index = menu_items.items.size();
   for (size_t i = index; i < menu_items.items.size(); i++) {
@@ -590,59 +592,6 @@ extensions::LaunchType AppServiceShelfContextMenu::GetExtensionLaunchType()
       extensions::ExtensionPrefs::Get(controller()->profile()), extension);
 }
 
-bool AppServiceShelfContextMenu::ShouldAddPinMenu() {
-  switch (app_type_) {
-    case apps::AppType::kArc: {
-      const arc::ArcAppShelfId& arc_shelf_id =
-          arc::ArcAppShelfId::FromString(item().id.app_id);
-      DCHECK(arc_shelf_id.valid());
-      const ArcAppListPrefs* arc_list_prefs =
-          ArcAppListPrefs::Get(controller()->profile());
-      DCHECK(arc_list_prefs);
-      std::unique_ptr<ArcAppListPrefs::AppInfo> app_info =
-          arc_list_prefs->GetApp(arc_shelf_id.app_id());
-      if (!arc_shelf_id.has_shelf_group_id() && app_info->launchable)
-        return true;
-      return false;
-    }
-    case apps::AppType::kPluginVm:
-    case apps::AppType::kBuiltIn: {
-      bool show_in_launcher = false;
-      apps::AppServiceProxyFactory::GetForProfile(controller()->profile())
-          ->AppRegistryCache()
-          .ForOneApp(item().id.app_id,
-                     [&show_in_launcher](const apps::AppUpdate& update) {
-                       show_in_launcher =
-                           update.ShowInLauncher().value_or(false);
-                     });
-      return show_in_launcher;
-    }
-    case apps::AppType::kCrostini:
-    case apps::AppType::kBorealis:
-    case apps::AppType::kChromeApp:
-    case apps::AppType::kWeb:
-    case apps::AppType::kSystemWeb:
-    case apps::AppType::kStandaloneBrowserChromeApp:
-      return true;
-    case apps::AppType::kStandaloneBrowser:
-      // Lacros behaves like the Chrome browser icon and cannot be unpinned.
-      return false;
-    case apps::AppType::kUnknown:
-      // Type kUnknown is used for "unregistered" Crostini apps, which do not
-      // have a .desktop file and can only be closed, not pinned.
-      return false;
-    case apps::AppType::kMacOs:
-    case apps::AppType::kRemote:
-    case apps::AppType::kExtension:
-    case apps::AppType::kStandaloneBrowserExtension:
-      NOTREACHED() << "Type " << (int)app_type_
-                   << " should not appear in shelf.";
-      return false;
-    case apps::AppType::kBruschetta:
-      return true;
-  }
-}
-
 void AppServiceShelfContextMenu::ExecutePublisherContextMenuCommand(
     int command_id) {
   DCHECK(command_id >= ash::LAUNCH_APP_SHORTCUT_FIRST &&

chrome/browser/ui/ash/shelf/app_service/app_service_shelf_context_menu.h

@@ -78,8 +78,6 @@ class AppServiceShelfContextMenu : public ShelfContextMenu {
   // Helpers to get the launch type for the extension item.
   extensions::LaunchType GetExtensionLaunchType() const;
 
-  bool ShouldAddPinMenu();
-
   void ExecutePublisherContextMenuCommand(int command_id);
 
   apps::AppType app_type_;

chrome/browser/ui/ash/shelf/chrome_shelf_controller.cc

@@ -40,6 +40,7 @@
 #include "chrome/browser/apps/app_service/extension_apps_utils.h"
 #include "chrome/browser/apps/icon_standardizer.h"
 #include "chrome/browser/ash/app_list/app_list_client_impl.h"
+#include "chrome/browser/ash/app_list/app_list_controller_delegate.h"
 #include "chrome/browser/ash/app_list/app_list_syncable_service_factory.h"
 #include "chrome/browser/ash/app_list/app_service/app_service_app_icon_loader.h"
 #include "chrome/browser/ash/app_list/arc/arc_app_utils.h"
@@ -1309,7 +1310,7 @@ void ChromeShelfController::UpdatePinnedAppsFromSync() {
       ++index;
   }
 
-  UpdatePolicyPinnedAppsFromPrefs();
+  UpdateAppsPinStatesFromPrefs();
 
   ReportUpdateShelfIconList(model_);
 }
@@ -1354,9 +1355,10 @@ bool ChromeShelfController::EnsureAppPinnedInModelAtIndex(
   return true;
 }
 
-void ChromeShelfController::UpdatePolicyPinnedAppsFromPrefs() {
-  for (int index = 0; index < model_->item_count(); index++)
+void ChromeShelfController::UpdateAppsPinStatesFromPrefs() {
+  for (int index = 0; index < model_->item_count(); index++) {
     UpdatePinnedByPolicyForItemAtIndex(index);
+  }
 }
 
 void ChromeShelfController::UpdatePinnedByPolicyForItemAtIndex(
@@ -1365,6 +1367,7 @@ void ChromeShelfController::UpdatePinnedByPolicyForItemAtIndex(
   const bool pinned_by_policy =
       GetPinnableForAppID(item.id.app_id, profile()) ==
       AppListControllerDelegate::PIN_FIXED;
+
   if (item.pinned_by_policy != pinned_by_policy) {
     item.pinned_by_policy = pinned_by_policy;
     model_->Set(model_index, item);
@@ -1373,6 +1376,23 @@ void ChromeShelfController::UpdatePinnedByPolicyForItemAtIndex(
   ReportUpdateShelfIconList(model_);
 }
 
+void ChromeShelfController::UpdateForcedPinStateForItemAtIndex(
+    int model_index) {
+  ash::ShelfItem item = model_->items()[model_index];
+  auto app_type = apps::AppServiceProxyFactory::GetForProfile(profile())
+                      ->AppRegistryCache()
+                      .GetAppType(item.id.app_id);
+
+  const bool pin_state_forced_by_type =
+      !IsAppPinEditable(app_type, item.id.app_id, profile());
+  if (item.pin_state_forced_by_type != pin_state_forced_by_type) {
+    item.pin_state_forced_by_type = pin_state_forced_by_type;
+    model_->Set(model_index, item);
+  }
+
+  ReportUpdateShelfIconList(model_);
+}
+
 ash::ShelfItemStatus ChromeShelfController::GetAppState(
     const std::string& app_id) {
   for (auto& it : web_contents_to_app_id_) {
@@ -1607,6 +1627,8 @@ void ChromeShelfController::ShelfItemAdded(int index) {
       model_->Set(index, item);
   }
 
+  UpdateForcedPinStateForItemAtIndex(index);
+
   // Update the pin position preference as needed.
   if (ShouldSyncItemWithReentrancy(item))
     SyncPinPosition(item.id);

chrome/browser/ui/ash/shelf/chrome_shelf_controller.h

@@ -371,13 +371,18 @@ class ChromeShelfController
   // Schedules re-sync of shelf model.
   void ScheduleUpdatePinnedAppsFromSync();
 
-  // Update the policy-pinned flag for each shelf item.
-  void UpdatePolicyPinnedAppsFromPrefs();
+  // Updates the policy-pinned and the forced-pin-state flag for each shelf
+  // item.
+  void UpdateAppsPinStatesFromPrefs();
 
   // Updates the policy-pinned flag for shelf item at `model_index` in shelf
   // model.
   void UpdatePinnedByPolicyForItemAtIndex(int model_index);
 
+  // Updates the pin_state_forced_by_type flag for shelf item at `model_index`
+  // in shelf model.
+  void UpdateForcedPinStateForItemAtIndex(int model_index);
+
   // Returns the shelf item status for the given |app_id|, which can be either
   // STATUS_RUNNING (if there is such an app) or STATUS_CLOSED.
   ash::ShelfItemStatus GetAppState(const std::string& app_id);

chrome/browser/ui/ash/shelf/chrome_shelf_controller_unittest.cc

@@ -2431,6 +2431,17 @@ TEST_F(ChromeShelfControllerTest, V1AppPinRunCloseUnpin) {
 
 // Test the V1 app interaction flow: run it, pin it, close it, unpin it.
 TEST_F(ChromeShelfControllerTest, V1AppRunPinCloseUnpin) {
+  // Set the app type of extension1_ to a chrome app, as the default unknown app
+  // type is not allowed to be pinned.
+  std::vector<apps::AppPtr> apps;
+  apps::AppPtr app =
+      std::make_unique<apps::App>(apps::AppType::kChromeApp, extension1_->id());
+  apps.push_back(std::move(app));
+  apps::AppServiceProxyFactory::GetForProfile(profile())
+      ->AppRegistryCache()
+      .OnApps(std::move(apps), apps::AppType::kChromeApp,
+              /*should_notify_initialized=*/false);
+
   InitShelfController();
 
   // The model should only contain the browser shortcut.