[Shortcut] Remove shortcuts.

This CL adds the flow to allow removing shortcut from the system.

BUG=1412708

Change-Id: I23792596dd4140acbb4cbc23bcee7bfa0d9565b2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4776802
Reviewed-by: Tim Sergeant <tsergeant@chromium.org>
Commit-Queue: Maggie Cai <mxcai@chromium.org>
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1185014}

chrome/browser/apps/app_service/publishers/shortcut_publisher.cc

@@ -25,4 +25,9 @@ void ShortcutPublisher::PublishShortcut(ShortcutPtr delta) {
   proxy_->ShortcutRegistryCache()->UpdateShortcut(std::move(delta));
 }
 
+void ShortcutPublisher::RemoveShortcut(const ShortcutId& id) {
+  CHECK(proxy_->ShortcutRegistryCache());
+  proxy_->ShortcutRegistryCache()->RemoveShortcut(id);
+}
+
 }  // namespace apps

chrome/browser/apps/app_service/publishers/shortcut_publisher.h

@@ -47,6 +47,9 @@ class ShortcutPublisher {
   // been called before the first call to this method.
   void PublishShortcut(ShortcutPtr delta);
 
+  // Remove shortcut represented by shortcut id `id`.
+  void RemoveShortcut(const ShortcutId& id);
+
   AppServiceProxy* proxy() { return proxy_; }
 
  private:

chrome/browser/apps/app_service/publishers/shortcut_publisher_unittest.cc

@@ -63,6 +63,10 @@ class FakeShortcutPublisher : public ShortcutPublisher {
     EXPECT_EQ(expected_display_id, display_id_);
   }
 
+  void RemoveShortcut(const ShortcutId& shortcut_id) {
+    ShortcutPublisher::RemoveShortcut(shortcut_id);
+  }
+
  private:
   bool shortcut_launched = false;
   std::string host_app_id_;
@@ -196,4 +200,37 @@ TEST_F(ShortcutPublisherTest, LaunchShortcut_CallsCorrectPublisher) {
       initial_web_app_shortcuts[0]->local_id, display_id);
 }
 
+TEST_F(ShortcutPublisherTest, RemoveShortcut) {
+  ShortcutPtr shortcut_1 = std::make_unique<Shortcut>("app_id_1", "local_id_1");
+  shortcut_1->name = "name1";
+
+  ShortcutPtr shortcut_2 = std::make_unique<Shortcut>("app_id_1", "local_id_2");
+  shortcut_2->name = "name2";
+  shortcut_2->shortcut_source = ShortcutSource::kDeveloper;
+
+  Shortcuts initial_chrome_shortcuts;
+  initial_chrome_shortcuts.push_back(std::move(shortcut_1));
+  initial_chrome_shortcuts.push_back(std::move(shortcut_2));
+
+  FakeShortcutPublisher fake_chrome_app_publisher(proxy(), AppType::kChromeApp,
+                                                  initial_chrome_shortcuts);
+
+  ShortcutRegistryCache* cache = proxy()->ShortcutRegistryCache();
+  ASSERT_TRUE(cache);
+
+  ASSERT_EQ(cache->GetAllShortcuts().size(), 2u);
+
+  EXPECT_TRUE(cache->HasShortcut(initial_chrome_shortcuts[0]->shortcut_id));
+  fake_chrome_app_publisher.RemoveShortcut(
+      initial_chrome_shortcuts[0]->shortcut_id);
+  EXPECT_EQ(cache->GetAllShortcuts().size(), 1u);
+  EXPECT_FALSE(cache->HasShortcut(initial_chrome_shortcuts[0]->shortcut_id));
+
+  EXPECT_TRUE(cache->HasShortcut(initial_chrome_shortcuts[1]->shortcut_id));
+  fake_chrome_app_publisher.RemoveShortcut(
+      initial_chrome_shortcuts[1]->shortcut_id);
+  EXPECT_EQ(cache->GetAllShortcuts().size(), 0u);
+  EXPECT_FALSE(cache->HasShortcut(initial_chrome_shortcuts[1]->shortcut_id));
+}
+
 }  // namespace apps

chrome/browser/ash/app_list/app_service/app_service_shortcut_model_builder.cc

@@ -16,7 +16,6 @@
 
 namespace {
 bool ShouldShowInLauncher(const apps::ShortcutSource& shortcut_source) {
-  // TODO(crbug.com/1412708): Add logic for shortcut removed.
   return shortcut_source == apps::ShortcutSource::kUser;
 }
 }  // namespace
@@ -62,6 +61,11 @@ void AppServiceShortcutModelBuilder::OnShortcutUpdated(
   }
 }
 
+void AppServiceShortcutModelBuilder::OnShortcutRemoved(
+    const apps::ShortcutId& id) {
+  RemoveApp(id.value(), false);
+}
+
 void AppServiceShortcutModelBuilder::OnShortcutRegistryCacheWillBeDestroyed(
     apps::ShortcutRegistryCache* cache) {
   shortcut_registry_cache_observation_.Reset();

chrome/browser/ash/app_list/app_service/app_service_shortcut_model_builder.h

@@ -33,6 +33,7 @@ class AppServiceShortcutModelBuilder
 
   // apps::ShortcutRegistryCache::Observer overrides:
   void OnShortcutUpdated(const apps::ShortcutUpdate& update) override;
+  void OnShortcutRemoved(const apps::ShortcutId& id) override;
   void OnShortcutRegistryCacheWillBeDestroyed(
       apps::ShortcutRegistryCache* cache) override;
 

chrome/browser/ash/app_list/app_service/app_service_shortcut_model_builder_unittest.cc

@@ -136,6 +136,26 @@ TEST_P(AppServiceShortcutModelBuilderTest, BuildModel) {
   EXPECT_EQ(model_updater()->ItemAtForTest(3)->name(), "Test 4");
 }
 
+TEST_P(AppServiceShortcutModelBuilderTest, RemoveShortcut) {
+  CreateBuilder(GetParam());
+  // Confirm there are 2 launcher shortcut items.
+  EXPECT_EQ(model_updater()->ItemCount(), 2u);
+  EXPECT_EQ(model_updater()->ItemAtForTest(0)->id(),
+            apps::GenerateShortcutId("host_app_id_1", "local_id_1").value());
+  EXPECT_EQ(model_updater()->ItemAtForTest(0)->name(), "Test 1");
+  EXPECT_EQ(model_updater()->ItemAtForTest(1)->id(),
+            apps::GenerateShortcutId("host_app_id_1", "local_id_2").value());
+  EXPECT_EQ(model_updater()->ItemAtForTest(1)->name(), "Test 2");
+
+  cache()->RemoveShortcut(
+      apps::ShortcutId(model_updater()->ItemAtForTest(0)->id()));
+  EXPECT_EQ(model_updater()->ItemCount(), 1u);
+
+  cache()->RemoveShortcut(
+      apps::ShortcutId(model_updater()->ItemAtForTest(0)->id()));
+  EXPECT_EQ(model_updater()->ItemCount(), 0u);
+}
+
 INSTANTIATE_TEST_SUITE_P(/*no prefix*/,
                          AppServiceShortcutModelBuilderTest,
                          testing::Values(true, false));

chrome/browser/web_applications/app_service/browser_shortcuts.cc

@@ -122,4 +122,14 @@ void BrowserShortcuts::OnWebAppInstallManagerDestroyed() {
   install_manager_observation_.Reset();
 }
 
+void BrowserShortcuts::OnWebAppUninstalled(
+    const AppId& app_id,
+    webapps::WebappUninstallSource uninstall_source) {
+  if (!IsShortcut(app_id)) {
+    return;
+  }
+  apps::ShortcutPublisher::RemoveShortcut(
+      apps::GenerateShortcutId(app_constants::kChromeAppId, app_id));
+}
+
 }  // namespace web_app

chrome/browser/web_applications/app_service/browser_shortcuts.h

@@ -56,6 +56,9 @@ class BrowserShortcuts : public apps::ShortcutPublisher,
   void OnWebAppInstalled(const AppId& app_id) override;
   void OnWebAppInstalledWithOsHooks(const AppId& app_id) override;
   void OnWebAppInstallManagerDestroyed() override;
+  void OnWebAppUninstalled(
+      const AppId& app_id,
+      webapps::WebappUninstallSource uninstall_source) override;
 
   const raw_ptr<Profile> profile_;
 

chrome/browser/web_applications/app_service/browser_shortcuts_unittest.cc

@@ -197,4 +197,26 @@ TEST_F(BrowserShortcutsTest, LaunchShortcut) {
   EXPECT_EQ(setting, LaunchWebAppWindowSetting::kUseLaunchParams);
 }
 
+TEST_F(BrowserShortcutsTest, RemoveShortcut) {
+  InitializeBrowserShortcutPublisher();
+  apps::ShortcutRegistryCache* cache =
+      apps::AppServiceProxyFactory::GetForProfile(profile())
+          ->ShortcutRegistryCache();
+  ASSERT_EQ(cache->GetAllShortcuts().size(), 0u);
+
+  const std::string kShortcutName = "Shortcut";
+
+  auto local_shortcut_id = CreateShortcut(kShortcutName);
+  apps::ShortcutId expected_shortcut_id =
+      apps::GenerateShortcutId(app_constants::kChromeAppId, local_shortcut_id);
+
+  ASSERT_EQ(cache->GetAllShortcuts().size(), 1u);
+  ASSERT_TRUE(cache->HasShortcut(expected_shortcut_id));
+
+  test::UninstallWebApp(profile(), local_shortcut_id);
+
+  EXPECT_EQ(cache->GetAllShortcuts().size(), 0u);
+  EXPECT_FALSE(cache->HasShortcut(expected_shortcut_id));
+}
+
 }  // namespace web_app

components/services/app_service/public/cpp/shortcut/shortcut_registry_cache.cc

@@ -33,7 +33,7 @@ void ShortcutRegistryCache::RemoveObserver(Observer* observer) {
 
 void ShortcutRegistryCache::UpdateShortcut(ShortcutPtr delta) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  // Do not allow notified observer updating shortcut cache again.
+  // Do not allow notified observer to modify shortcut cache again.
   DCHECK(!is_updating_);
   is_updating_ = true;
   const ShortcutId shortcut_id = delta->shortcut_id;
@@ -54,6 +54,18 @@ void ShortcutRegistryCache::UpdateShortcut(ShortcutPtr delta) {
   is_updating_ = false;
 }
 
+void ShortcutRegistryCache::RemoveShortcut(const ShortcutId& id) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  // Do not allow notified observer to modify shortcut cache again.
+  CHECK(!is_updating_);
+  is_updating_ = true;
+  states_.erase(id);
+  for (auto& obs : observers_) {
+    obs.OnShortcutRemoved(id);
+  }
+  is_updating_ = false;
+}
+
 ShortcutView ShortcutRegistryCache::GetShortcut(const ShortcutId& shortcut_id) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   auto shortcut = states_.find(shortcut_id);

components/services/app_service/public/cpp/shortcut/shortcut_registry_cache.h

@@ -29,6 +29,9 @@ class COMPONENT_EXPORT(SHORTCUT) ShortcutRegistryCache {
     // has been updated and what changes have been made.
     virtual void OnShortcutUpdated(const ShortcutUpdate& update) {}
 
+    // Called when a shortcut represented by `id` been removed from the system.
+    virtual void OnShortcutRemoved(const ShortcutId& id) {}
+
     // Called when the ShortcutRegistryCache object (the thing that this
     // observer observes) will be destroyed. In response, the observer, |this|,
     // should call "cache->RemoveObserver(this)", whether directly or indirectly
@@ -51,7 +54,8 @@ class COMPONENT_EXPORT(SHORTCUT) ShortcutRegistryCache {
   // shortcut if it doesn't exists.
   void UpdateShortcut(ShortcutPtr delta);
 
-  // TODO(crbug.com/1412708): Add remove flow.
+  // Removes the shortcut represented by `id` from the cache.
+  void RemoveShortcut(const ShortcutId& id);
 
   // Get the shortcut by the id, return nullptr if shortcut id doesn't exist.
   // Be careful about the lifetime when using this method, the ShortcutView is

components/services/app_service/public/cpp/shortcut/shortcut_registry_cache_unittest.cc

@@ -28,19 +28,35 @@ class ShortcutRegistryCacheTest : public testing::Test,
   }
   bool OnShortcutUpdatedCalled() { return on_shortcut_updated_called_; }
 
+  void ExpectShortcutRemoved(const ShortcutId& shortcut_id) {
+    expected_shortcut_id_ = shortcut_id;
+    if (!obs_.IsObserving()) {
+      obs_.Observe(&cache());
+    }
+    on_shortcut_removed_called_ = false;
+  }
+  bool OnShortcutRemovedCalled() { return on_shortcut_removed_called_; }
+
  private:
   void OnShortcutUpdated(const ShortcutUpdate& update) override {
     on_shortcut_updated_called_ = true;
     EXPECT_EQ(update, *expected_update_);
   }
 
+  void OnShortcutRemoved(const ShortcutId& shortcut_id) override {
+    on_shortcut_removed_called_ = true;
+    EXPECT_EQ(shortcut_id, expected_shortcut_id_);
+  }
+
   void OnShortcutRegistryCacheWillBeDestroyed(
       ShortcutRegistryCache* cache) override {
     obs_.Reset();
   }
   ShortcutRegistryCache cache_;
   std::unique_ptr<ShortcutUpdate> expected_update_;
+  ShortcutId expected_shortcut_id_;
   bool on_shortcut_updated_called_ = false;
+  bool on_shortcut_removed_called_ = false;
   base::ScopedObservation<ShortcutRegistryCache,
                           ShortcutRegistryCache::Observer>
       obs_{this};
@@ -102,6 +118,24 @@ TEST_F(ShortcutRegistryCacheTest, UpdateShortcut) {
   EXPECT_EQ(stored_shortcut->local_id, local_id);
 }
 
+TEST_F(ShortcutRegistryCacheTest, RemoveShortcut) {
+  std::string host_app_id = "host_app_id";
+  std::string local_id = "local_id";
+  auto shortcut = std::make_unique<Shortcut>(host_app_id, local_id);
+  ShortcutId shortcut_id = shortcut->shortcut_id;
+  shortcut->name = "name";
+  shortcut->shortcut_source = ShortcutSource::kUser;
+
+  cache().UpdateShortcut(std::move(shortcut));
+  ASSERT_TRUE(cache().HasShortcut(shortcut_id));
+  ASSERT_EQ(cache().GetAllShortcuts().size(), 1u);
+
+  cache().RemoveShortcut(shortcut_id);
+
+  EXPECT_EQ(cache().GetAllShortcuts().size(), 0u);
+  EXPECT_FALSE(cache().HasShortcut(shortcut_id));
+}
+
 TEST_F(ShortcutRegistryCacheTest, Observer) {
   std::string host_app_id = "host_app_id";
   std::string local_id = "local_id";
@@ -130,5 +164,10 @@ TEST_F(ShortcutRegistryCacheTest, Observer) {
       current_state.get(), shortcut_nochange.get()));
   cache().UpdateShortcut(std::move(shortcut_nochange));
   EXPECT_TRUE(OnShortcutUpdatedCalled());
+
+  ExpectShortcutRemoved(shortcut_id);
+  cache().RemoveShortcut(shortcut_id);
+  EXPECT_TRUE(OnShortcutRemovedCalled());
 }
+
 }  // namespace apps