Add RemoveIcon to IconCache to cache icons for the search result.

When clicks the launcher button, AppServiceAppResult is created for each
app, and call LoadIcon, which could genearte mojom calls to load the
icon.

Remove SweepReleasedIcons when the view is closed to keep the icon
cache in IconCache. When the app is removed or the icon is updated, call
RemoveIcon to clear the map in IconCache.

BUG=1104650

Change-Id: I6f5bfb44c9892187976f89ecb3993430fb565f2f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2318486
Commit-Queue: Nancy Wang <nancylingwang@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Cr-Commit-Position: refs/heads/master@{#791922}

chrome/browser/ui/app_list/search/app_search_provider.cc

@@ -378,11 +378,14 @@ class AppServiceDataSource : public AppSearchProvider::DataSource,
         profile(), app_id, list_controller, is_recommended, &icon_cache_);
   }
 
-  void ViewClosing() override { icon_cache_.SweepReleasedIcons(); }
-
  private:
   // apps::AppRegistryCache::Observer overrides:
   void OnAppUpdate(const apps::AppUpdate& update) override {
+    if (update.Readiness() == apps::mojom::Readiness::kUninstalledByUser ||
+        update.IconKeyChanged()) {
+      icon_cache_.RemoveIcon(update.AppType(), update.AppId());
+    }
+
     if (update.Readiness() == apps::mojom::Readiness::kReady) {
       owner()->RefreshAppsAndUpdateResultsDeferred();
     } else {

chrome/browser/ui/app_list/search/tests/app_search_provider_unittest.cc

@@ -43,6 +43,7 @@
 #include "components/arc/test/fake_app_instance.h"
 #include "components/crx_file/id_util.h"
 #include "components/services/app_service/public/cpp/stub_icon_loader.h"
+#include "components/services/app_service/public/mojom/types.mojom.h"
 #include "components/sessions/content/content_test_helper.h"
 #include "components/sessions/core/serialized_navigation_entry_test_helper.h"
 #include "components/sessions/core/session_id.h"
@@ -109,6 +110,23 @@ bool MoreRelevant(const ChromeSearchResult* result1,
   return result1->relevance() > result2->relevance();
 }
 
+void UpdateIconKey(apps::AppServiceProxy& proxy, const std::string& app_id) {
+  apps::mojom::AppPtr app = apps::mojom::App::New();
+  app->app_id = app_id;
+  proxy.AppRegistryCache().ForOneApp(
+      app_id, [&app](const apps::AppUpdate& update) {
+        app->app_type = update.AppType();
+        app->icon_key = apps::mojom::IconKey::New(
+            update.IconKey()->timeline + 1, update.IconKey()->resource_id,
+            update.IconKey()->icon_effects);
+      });
+
+  std::vector<apps::mojom::AppPtr> apps;
+  apps.push_back(app.Clone());
+  proxy.AppRegistryCache().OnApps(std::move(apps));
+  proxy.FlushMojoCallsForTesting();
+}
+
 class AppSearchProviderTest : public AppListTestBase {
  public:
   AppSearchProviderTest() {
@@ -835,15 +853,23 @@ TEST_F(AppSearchProviderTest, AppServiceIconCache) {
   RunQuery("pa");
   EXPECT_EQ(2, stub_icon_loader.NumLoadIconFromIconKeyCalls());
 
-  // Hiding the UI (i.e. calling ViewClosing) should clear the icon cache. The
-  // number of LoadIconFromIconKey calls should not change.
+  // The number of LoadIconFromIconKey calls should not change, when hiding the
+  // UI (i.e. calling ViewClosing).
   CallViewClosing();
   EXPECT_EQ(2, stub_icon_loader.NumLoadIconFromIconKeyCalls());
 
-  // Issuing the same "pa" query should bypass the now-clear icon cache, with 2
-  // further calls to the wrapped stub_icon_loader, bringing the total to 4.
+  // The icon has been added to the map, so issuing the same "pa" query should
+  // not call the wrapped stub_icon_loader.
+  RunQuery("pa");
+  EXPECT_EQ(2, stub_icon_loader.NumLoadIconFromIconKeyCalls());
+
+  // Update the icon key to remove the app icon from cache.
+  UpdateIconKey(*proxy, kPackagedApp2Id);
+
+  // The icon has been removed from the cache, so issuing the same "pa" query
+  // should call the wrapped stub_icon_loader.
   RunQuery("pa");
-  EXPECT_EQ(4, stub_icon_loader.NumLoadIconFromIconKeyCalls());
+  EXPECT_EQ(3, stub_icon_loader.NumLoadIconFromIconKeyCalls());
 
   proxy->OverrideInnerIconLoaderForTesting(old_icon_loader);
 }

components/services/app_service/public/cpp/icon_cache.cc

@@ -109,6 +109,23 @@ void IconCache::SweepReleasedIcons() {
   }
 }
 
+void IconCache::RemoveIcon(apps::mojom::AppType app_type,
+                           const std::string& app_id) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  if (gc_policy_ != GarbageCollectionPolicy::kExplicit) {
+    return;
+  }
+
+  auto iter = map_.begin();
+  while (iter != map_.end()) {
+    if (iter->first.app_type_ == app_type && iter->first.app_id_ == app_id) {
+      iter = map_.erase(iter);
+    } else {
+      ++iter;
+    }
+  }
+}
+
 void IconCache::Update(const IconLoader::Key& key,
                        const apps::mojom::IconValue& icon_value) {
   if (icon_value.icon_type != apps::mojom::IconType::kUncompressed &&
@@ -143,13 +160,13 @@ void IconCache::OnRelease(IconLoader::Key key) {
 
   auto iter = map_.find(key);
   if (iter == map_.end()) {
-    NOTREACHED();
     return;
   }
 
   auto n = iter->second.ref_count_;
-  CHECK(n > 0);
-  n--;
+  if (n > 0) {
+    n--;
+  }
   iter->second.ref_count_ = n;
 
   if ((n == 0) && (gc_policy_ == GarbageCollectionPolicy::kEager)) {

components/services/app_service/public/cpp/icon_cache.h

@@ -84,6 +84,8 @@ class IconCache : public IconLoader {
   // actively held.
   void SweepReleasedIcons();
 
+  void RemoveIcon(apps::mojom::AppType app_type, const std::string& app_id);
+
  private:
   class Value {
    public:

components/services/app_service/public/cpp/icon_cache_unittest.cc

@@ -81,7 +81,8 @@ class AppsIconCacheTest : public testing::Test {
     return releaser;
   }
 
-  void TestBasics(apps::IconCache::GarbageCollectionPolicy gc_policy) {
+  void TestBasics(apps::IconCache::GarbageCollectionPolicy gc_policy,
+                  bool remove_icon = false) {
     FakeIconLoader fake;
     apps::IconCache cache(&fake, gc_policy);
 
@@ -105,16 +106,27 @@ class AppsIconCacheTest : public testing::Test {
     UniqueReleaser c3 = LoadIcon(&cache, &fake, "cherry", kHit);
     c3.reset();
 
+    HitOrMiss expect_hom = kHit;
     if (gc_policy == apps::IconCache::GarbageCollectionPolicy::kExplicit) {
-      cache.SweepReleasedIcons();
+      if (remove_icon) {
+        cache.RemoveIcon(apps::mojom::AppType::kWeb, "cherry");
+        cache.RemoveIcon(apps::mojom::AppType::kWeb, "apricot");
+        expect_hom = kMiss;
+      } else {
+        cache.SweepReleasedIcons();
+      }
     }
 
-    UniqueReleaser c4 = LoadIcon(&cache, &fake, "cherry", kHit);
+    UniqueReleaser c4 = LoadIcon(&cache, &fake, "cherry", expect_hom);
     c4.reset();
     c0.reset();
 
     if (gc_policy == apps::IconCache::GarbageCollectionPolicy::kExplicit) {
-      cache.SweepReleasedIcons();
+      if (remove_icon) {
+        cache.RemoveIcon(apps::mojom::AppType::kWeb, "cherry");
+      } else {
+        cache.SweepReleasedIcons();
+      }
     }
 
     UniqueReleaser c5 = LoadIcon(&cache, &fake, "cherry", kMiss);
@@ -157,8 +169,8 @@ class AppsIconCacheTest : public testing::Test {
         LoadIcon(&cache, &fake, "fig", kHit, allow_placeholder_icon);
   }
 
-  void TestAfterZeroRefcount(
-      apps::IconCache::GarbageCollectionPolicy gc_policy) {
+  void TestAfterZeroRefcount(apps::IconCache::GarbageCollectionPolicy gc_policy,
+                             bool remove_icon = false) {
     FakeIconLoader fake;
     apps::IconCache cache(&fake, gc_policy);
 
@@ -186,7 +198,11 @@ class AppsIconCacheTest : public testing::Test {
     // get kMiss.
 
     if (gc_policy == apps::IconCache::GarbageCollectionPolicy::kExplicit) {
-      cache.SweepReleasedIcons();
+      if (remove_icon) {
+        cache.RemoveIcon(apps::mojom::AppType::kWeb, "watermelon");
+      } else {
+        cache.SweepReleasedIcons();
+      }
     }
 
     UniqueReleaser w2 = LoadIcon(&cache, &fake, "watermelon", kMiss);
@@ -203,7 +219,7 @@ TEST_F(AppsIconCacheTest, Eager) {
   TestAfterZeroRefcount(gc_policy);
 }
 
-TEST_F(AppsIconCacheTest, Explicit) {
+TEST_F(AppsIconCacheTest, ExplicitSweepReleasedIcons) {
   static constexpr apps::IconCache::GarbageCollectionPolicy gc_policy =
       apps::IconCache::GarbageCollectionPolicy::kExplicit;
 
@@ -211,3 +227,12 @@ TEST_F(AppsIconCacheTest, Explicit) {
   TestPlaceholder(gc_policy);
   TestAfterZeroRefcount(gc_policy);
 }
+
+TEST_F(AppsIconCacheTest, ExplicitRemoveIcons) {
+  static constexpr apps::IconCache::GarbageCollectionPolicy gc_policy =
+      apps::IconCache::GarbageCollectionPolicy::kExplicit;
+
+  TestBasics(gc_policy, true /* remove_icon */);
+  TestPlaceholder(gc_policy);
+  TestAfterZeroRefcount(gc_policy, true /* remove_icon */);
+}