Omnibox: Update FaviconCache to observe favicon changed notifications

Previously we were timing out 'null' favicon results after 60 seconds.

This has turned out to be too simple of a heuristic now that we are
using the FaviconCache to back the default search provider icon in the
location bar.

Users would expect that the favicon appear immediately after loading a
matching search results page, and would be confused by a 60 second
delay.

Bug: 88243, 823535
Change-Id: I062361ffad0e0bdaa45f44ad67a7071efa086f2c
Reviewed-on: https://chromium-review.googlesource.com/1055787
Commit-Queue: Tommy Li <tommycli@chromium.org>
Reviewed-by: Justin Donnelly <jdonnelly@chromium.org>
Cr-Commit-Position: refs/heads/master@{#558816}

components/omnibox/browser/favicon_cache.cc

@@ -21,9 +21,6 @@ size_t GetFaviconCacheSize() {
 
 }  // namespace
 
-// static
-const int FaviconCache::kEmptyFaviconCacheLifetimeInSeconds = 60;
-
 bool FaviconCache::Request::operator<(const Request& rhs) const {
   // Compare |type| first, and if equal, compare |url|.
   return std::tie(type, url) < std::tie(rhs.type, rhs.url);
@@ -36,8 +33,13 @@ FaviconCache::FaviconCache(favicon::FaviconService* favicon_service,
       mru_cache_(GetFaviconCacheSize()),
       responses_without_favicons_(GetFaviconCacheSize()),
       weak_factory_(this) {
-  if (history_service)
+  if (history_service) {
     history_observer_.Add(history_service);
+
+    favicons_changed_subscription_ =
+        history_service->AddFaviconsChangedCallback(base::BindRepeating(
+            &FaviconCache::OnFaviconsChanged, weak_factory_.GetWeakPtr()));
+  }
 }
 
 FaviconCache::~FaviconCache() {}
@@ -71,7 +73,6 @@ gfx::Image FaviconCache::GetFaviconInternal(
     return cache_iterator->second;
 
   // Early exit if we've already established that we don't have the favicon.
-  AgeOutOldCachedEmptyFavicons();
   if (responses_without_favicons_.Peek(request) !=
       responses_without_favicons_.end()) {
     return gfx::Image();
@@ -110,7 +111,7 @@ void FaviconCache::OnFaviconFetched(
     const Request& request,
     const favicon_base::FaviconImageResult& result) {
   if (result.image.IsEmpty()) {
-    responses_without_favicons_.Put(request, GetTimeNow());
+    responses_without_favicons_.Put(request, true);
     pending_requests_.erase(request);
     return;
   }
@@ -136,21 +137,18 @@ void FaviconCache::OnURLVisited(history::HistoryService* history_service,
     responses_without_favicons_.Erase(it);
 }
 
-void FaviconCache::AgeOutOldCachedEmptyFavicons() {
-  // TODO(tommycli): This code is based on chrome_browser_net::TimedCache.
-  // Investigate combining.
-  base::TimeDelta max_duration =
-      base::TimeDelta::FromSeconds(kEmptyFaviconCacheLifetimeInSeconds);
-  base::TimeTicks age_cutoff = GetTimeNow() - max_duration;
-  auto eldest = responses_without_favicons_.rbegin();
-  while (eldest != responses_without_favicons_.rend() &&
-         eldest->second <= age_cutoff) {
-    eldest = responses_without_favicons_.Erase(eldest);
+void FaviconCache::InvalidateCachedRequests(const Request& request) {
+  {
+    auto it = mru_cache_.Peek(request);
+    if (it != mru_cache_.end())
+      mru_cache_.Erase(it);
   }
-}
 
-base::TimeTicks FaviconCache::GetTimeNow() {
-  return base::TimeTicks::Now();
+  {
+    auto it = responses_without_favicons_.Peek(request);
+    if (it != responses_without_favicons_.end())
+      responses_without_favicons_.Erase(it);
+  }
 }
 
 void FaviconCache::OnURLsDeleted(history::HistoryService* history_service,
@@ -161,12 +159,20 @@ void FaviconCache::OnURLsDeleted(history::HistoryService* history_service,
 
   if (deletion_info.IsAllHistory()) {
     mru_cache_.Clear();
+    responses_without_favicons_.Clear();
     return;
   }
 
   for (const history::URLRow& row : deletion_info.deleted_rows()) {
-    auto it = mru_cache_.Peek({RequestType::BY_PAGE_URL, row.url()});
-    if (it != mru_cache_.end())
-      mru_cache_.Erase(it);
+    InvalidateCachedRequests({RequestType::BY_PAGE_URL, row.url()});
+  }
+}
+
+void FaviconCache::OnFaviconsChanged(const std::set<GURL>& page_urls,
+                                     const GURL& icon_url) {
+  for (GURL page_url : page_urls) {
+    InvalidateCachedRequests({RequestType::BY_PAGE_URL, page_url});
   }
+
+  InvalidateCachedRequests({RequestType::BY_ICON_URL, icon_url});
 }

components/omnibox/browser/favicon_cache.h

@@ -9,13 +9,13 @@
 #include <map>
 
 #include "base/callback_forward.h"
+#include "base/callback_list.h"
 #include "base/containers/mru_cache.h"
 #include "base/gtest_prod_util.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "base/scoped_observer.h"
 #include "base/task/cancelable_task_tracker.h"
-#include "base/time/time.h"
 #include "components/favicon_base/favicon_types.h"
 #include "components/history/core/browser/history_service_observer.h"
 #include "components/history/core/browser/history_types.h"
@@ -37,8 +37,8 @@ typedef base::OnceCallback<void(const gfx::Image& favicon)>
 // of them so we can synchronously deliver them to the UI to prevent flicker as
 // the user types.
 //
-// It also stores and times out null results from when we cannot fetch a favicon
-// from the history database. Null results timeout after a while.
+// This class also observes the HistoryService, and invalidates cached favicons
+// and null responses when matching favicons are updated.
 class FaviconCache : public history::HistoryServiceObserver {
  public:
   FaviconCache(favicon::FaviconService* favicon_service,
@@ -73,7 +73,7 @@ class FaviconCache : public history::HistoryServiceObserver {
  private:
   FRIEND_TEST_ALL_PREFIXES(FaviconCacheTest, ClearIconsWithHistoryDeletions);
   FRIEND_TEST_ALL_PREFIXES(FaviconCacheTest, ExpireNullFaviconsByHistory);
-  FRIEND_TEST_ALL_PREFIXES(FaviconCacheTest, ExpireNullFaviconsByTime);
+  FRIEND_TEST_ALL_PREFIXES(FaviconCacheTest, ObserveFaviconsChanged);
 
   enum class RequestType {
     BY_PAGE_URL,
@@ -88,9 +88,6 @@ class FaviconCache : public history::HistoryServiceObserver {
     bool operator<(const Request& rhs) const;
   };
 
-  // Chosen arbitrarily. Declared in the class for testing.
-  static const int kEmptyFaviconCacheLifetimeInSeconds;
-
   // Internal method backing GetFaviconForPageUrl and GetFaviconForIconUrl.
   gfx::Image GetFaviconInternal(const Request& request,
                                 FaviconFetchedCallback on_favicon_fetched);
@@ -100,10 +97,9 @@ class FaviconCache : public history::HistoryServiceObserver {
   void OnFaviconFetched(const Request& request,
                         const favicon_base::FaviconImageResult& result);
 
-  void AgeOutOldCachedEmptyFavicons();
-
-  // Virtual for testing.
-  virtual base::TimeTicks GetTimeNow();
+  // Removes cached favicons and null responses that match |request| from the
+  // cache. Subsequent matching requests pull fresh data from FaviconService.
+  void InvalidateCachedRequests(const Request& request);
 
   // history::HistoryServiceObserver:
   void OnURLVisited(history::HistoryService* history_service,
@@ -113,6 +109,7 @@ class FaviconCache : public history::HistoryServiceObserver {
                     base::Time visit_time) override;
   void OnURLsDeleted(history::HistoryService* history_service,
                      const history::DeletionInfo& deletion_info) override;
+  void OnFaviconsChanged(const std::set<GURL>& page_urls, const GURL& icon_url);
 
   // Non-owning pointer to a KeyedService.
   favicon::FaviconService* favicon_service_;
@@ -126,8 +123,13 @@ class FaviconCache : public history::HistoryServiceObserver {
 
   // Keep responses with empty favicons in a separate list, to prevent a
   // response with an empty favicon from ever evicting an existing favicon.
-  // The value is used to age out entries that are too old.
-  base::MRUCache<Request, base::TimeTicks> responses_without_favicons_;
+  // The value is always set to true and has no meaning.
+  base::MRUCache<Request, bool> responses_without_favicons_;
+
+  // Subscription for notifications of changes to favicons.
+  std::unique_ptr<base::CallbackList<void(const std::set<GURL>&,
+                                          const GURL&)>::Subscription>
+      favicons_changed_subscription_;
 
   base::WeakPtrFactory<FaviconCache> weak_factory_;
 

components/omnibox/browser/favicon_cache_unittest.cc

@@ -44,19 +44,6 @@ void Fail(const gfx::Image& favicon) {
   FAIL() << "This asynchronous callback should never have been called.";
 }
 
-class TestFaviconCache : public FaviconCache {
- public:
-  explicit TestFaviconCache(favicon::FaviconService* favicon_service)
-      : FaviconCache(favicon_service, nullptr /* history_service */) {}
-  ~TestFaviconCache() override {}
-
-  base::TimeTicks GetTimeNow() override { return fake_time_now_; }
-  void SetTimeNow(base::TimeTicks now) { fake_time_now_ = now; }
-
- private:
-  base::TimeTicks fake_time_now_;
-};
-
 }  // namespace
 
 class FaviconCacheTest : public testing::Test {
@@ -65,7 +52,8 @@ class FaviconCacheTest : public testing::Test {
   const GURL kUrlB = GURL("http://www.b.com/");
   const GURL kIconUrl = GURL("http://a.com/favicon.ico");
 
-  FaviconCacheTest() : cache_(&favicon_service_) {}
+  FaviconCacheTest()
+      : cache_(&favicon_service_, nullptr /* history_service */) {}
 
   testing::NiceMock<favicon::MockFaviconService> favicon_service_;
 
@@ -100,7 +88,7 @@ class FaviconCacheTest : public testing::Test {
   favicon_base::FaviconImageCallback favicon_service_a_site_response_;
   favicon_base::FaviconImageCallback favicon_service_b_site_response_;
 
-  TestFaviconCache cache_;
+  FaviconCache cache_;
 };
 
 TEST_F(FaviconCacheTest, Basic) {
@@ -275,7 +263,7 @@ TEST_F(FaviconCacheTest, ExpireNullFaviconsByHistory) {
       cache_.GetFaviconForPageUrl(kUrlA, base::BindOnce(&Fail)).IsEmpty());
 }
 
-TEST_F(FaviconCacheTest, ExpireNullFaviconsByTime) {
+TEST_F(FaviconCacheTest, ObserveFaviconsChanged) {
   ExpectFaviconServiceForPageUrlCalls(2, 1);
 
   EXPECT_TRUE(
@@ -283,21 +271,11 @@ TEST_F(FaviconCacheTest, ExpireNullFaviconsByTime) {
   EXPECT_TRUE(
       cache_.GetFaviconForPageUrl(kUrlB, base::BindOnce(&Fail)).IsEmpty());
 
-  base::TimeDelta max_duration = base::TimeDelta::FromSeconds(
-      FaviconCache::kEmptyFaviconCacheLifetimeInSeconds);
-
-  // Set the time to epoch and respond with the first empty favicon.
-  cache_.SetTimeNow(base::TimeTicks());
+  // Simulate responses to both requests.
   favicon_service_a_site_response_.Run(favicon_base::FaviconImageResult());
-
-  // Increment the time to half the duration of the expiry time and respond
-  // with the second empty favicon.
-  cache_.SetTimeNow(base::TimeTicks() + max_duration / 2);
   favicon_service_b_site_response_.Run(favicon_base::FaviconImageResult());
 
-  // Now increment the time so the the first, but not the second, cached empty
-  // favicon is expired.
-  cache_.SetTimeNow(base::TimeTicks() + max_duration);
+  cache_.OnFaviconsChanged({kUrlA}, GURL());
 
   // Request the two favicons again.
   EXPECT_TRUE(
@@ -307,5 +285,5 @@ TEST_F(FaviconCacheTest, ExpireNullFaviconsByTime) {
 
   // Our call to |ExpectFaviconServiceForPageUrlCalls(expected A calls, expected
   // B calls)| above should verify that we re-request the icon for kUrlA only
-  // (because the empty result has been aged out).
+  // (because the null result has been invalidated by OnFaviconsChanged).
 }