[EoC] Cache suggestions fetch results

Bug: 830919
Change-Id: I1fd9bdbd5f15003d1006518b6883111b5c6e6a5e
Reviewed-on: https://chromium-review.googlesource.com/1043095
Commit-Queue: Patrick Noland <pnoland@chromium.org>
Reviewed-by: Filip Gorski <fgorski@chromium.org>
Cr-Commit-Position: refs/heads/master@{#561921}

components/ntp_snippets/BUILD.gn

@@ -52,6 +52,8 @@ static_library("ntp_snippets") {
     "contextual/contextual_content_suggestions_service_proxy.h",
     "contextual/contextual_suggestion.cc",
     "contextual/contextual_suggestion.h",
+    "contextual/contextual_suggestions_cache.cc",
+    "contextual/contextual_suggestions_cache.h",
     "contextual/contextual_suggestions_composite_reporter.cc",
     "contextual/contextual_suggestions_composite_reporter.h",
     "contextual/contextual_suggestions_debugging_reporter.cc",

components/ntp_snippets/contextual/contextual_content_suggestions_service.cc

@@ -11,11 +11,11 @@
 
 #include "base/bind.h"
 #include "base/memory/ptr_util.h"
+#include "components/ntp_snippets/contextual/contextual_content_suggestions_service_proxy.h"
 #include "components/ntp_snippets/contextual/contextual_suggestions_result.h"
 #include "components/ntp_snippets/remote/cached_image_fetcher.h"
 #include "components/ntp_snippets/remote/remote_suggestions_database.h"
 #include "components/ntp_snippets/remote/remote_suggestions_provider_impl.h"
-#include "contextual_content_suggestions_service_proxy.h"
 #include "ui/gfx/image/image.h"
 
 namespace contextual_suggestions {
@@ -43,6 +43,7 @@ ContextualContentSuggestionsService::ContextualContentSuggestionsService(
     std::unique_ptr<ContextualSuggestionsReporterProvider> reporter_provider)
     : contextual_suggestions_database_(
           std::move(contextual_suggestions_database)),
+      fetch_cache_(kFetchCacheCapacity),
       contextual_suggestions_fetcher_(
           std::move(contextual_suggestions_fetcher)),
       image_fetcher_(std::move(image_fetcher)),
@@ -56,15 +57,15 @@ void ContextualContentSuggestionsService::FetchContextualSuggestionClusters(
     FetchClustersCallback callback,
     ReportFetchMetricsCallback metrics_callback) {
   // TODO(pnoland): Also check that the url is safe.
-  if (IsEligibleURL(url)) {
+  ContextualSuggestionsResult result;
+  if (IsEligibleURL(url) && !fetch_cache_.GetSuggestionsResult(url, &result)) {
     FetchClustersCallback internal_callback = base::BindOnce(
         &ContextualContentSuggestionsService::FetchDone, base::Unretained(this),
-        std::move(callback), metrics_callback);
+        url, std::move(callback), metrics_callback);
     contextual_suggestions_fetcher_->FetchContextualSuggestionsClusters(
         url, std::move(internal_callback), metrics_callback);
   } else {
-    std::move(callback).Run(
-        ContextualSuggestionsResult("", {}, PeekConditions()));
+    std::move(callback).Run(result);
   }
 }
 
@@ -78,9 +79,16 @@ void ContextualContentSuggestionsService::FetchContextualSuggestionImage(
 }
 
 void ContextualContentSuggestionsService::FetchDone(
+    const GURL& url,
     FetchClustersCallback callback,
     ReportFetchMetricsCallback metrics_callback,
     ContextualSuggestionsResult result) {
+  // We still want to cache low confidence results so that we avoid doing
+  // unnecessary fetches.
+  if (result.clusters.size() > 0) {
+    fetch_cache_.AddSuggestionsResult(url, result);
+  }
+
   if (result.peek_conditions.confidence < kMinimumConfidence) {
     metrics_callback.Run(contextual_suggestions::FETCH_BELOW_THRESHOLD);
     std::move(callback).Run(

components/ntp_snippets/contextual/contextual_content_suggestions_service.h

@@ -16,6 +16,7 @@
 #include "components/keyed_service/core/keyed_service.h"
 #include "components/ntp_snippets/callbacks.h"
 #include "components/ntp_snippets/content_suggestion.h"
+#include "components/ntp_snippets/contextual/contextual_suggestions_cache.h"
 #include "components/ntp_snippets/contextual/contextual_suggestions_fetcher.h"
 #include "components/ntp_snippets/contextual/contextual_suggestions_reporter.h"
 #include "services/metrics/public/cpp/ukm_source_id.h"
@@ -27,6 +28,8 @@ class RemoteSuggestionsDatabase;
 
 namespace contextual_suggestions {
 
+static constexpr int kFetchCacheCapacity = 10;
+
 class ContextualContentSuggestionsServiceProxy;
 
 // Retrieves contextual suggestions for a given URL and fetches images
@@ -57,7 +60,8 @@ class ContextualContentSuggestionsService : public KeyedService {
       const GURL& url,
       ntp_snippets::ImageFetchedCallback callback);
 
-  void FetchDone(FetchClustersCallback callback,
+  void FetchDone(const GURL& url,
+                 FetchClustersCallback callback,
                  ReportFetchMetricsCallback metrics_callback,
                  ContextualSuggestionsResult result);
 
@@ -68,6 +72,9 @@ class ContextualContentSuggestionsService : public KeyedService {
   std::unique_ptr<ntp_snippets::RemoteSuggestionsDatabase>
       contextual_suggestions_database_;
 
+  // Cache of contextual suggestions fetch results, keyed by the context url.
+  ContextualSuggestionsCache fetch_cache_;
+
   // Performs actual network request to fetch contextual suggestions.
   std::unique_ptr<ContextualSuggestionsFetcher> contextual_suggestions_fetcher_;
 

components/ntp_snippets/contextual/contextual_content_suggestions_service_proxy_unittest.cc

@@ -6,6 +6,7 @@
 
 #include <memory>
 #include <string>
+#include <utility>
 
 #include "base/bind.h"
 #include "components/ntp_snippets/contextual/contextual_content_suggestions_service.h"
@@ -23,8 +24,8 @@ namespace contextual_suggestions {
 
 namespace {
 
-static const std::string kTestPeekText("Test peek test");
-static const std::string kValidFromUrl = "http://some.url";
+static constexpr char kTestPeekText[] = "Test peek test";
+static constexpr char kValidFromUrl[] = "http://some.url";
 
 class FakeContextualContentSuggestionsService
     : public ContextualContentSuggestionsService {
@@ -47,14 +48,13 @@ class FakeContextualContentSuggestionsService
   FetchClustersCallback clusters_callback_;
 };
 
-}  // namespace
-
 FakeContextualContentSuggestionsService::
     FakeContextualContentSuggestionsService()
     : ContextualContentSuggestionsService(nullptr, nullptr, nullptr, nullptr) {}
 
 FakeContextualContentSuggestionsService::
     ~FakeContextualContentSuggestionsService() {}
+}  // namespace
 
 class ContextualContentSuggestionsServiceProxyTest : public testing::Test {
  public:
@@ -83,7 +83,9 @@ TEST_F(ContextualContentSuggestionsServiceProxyTest,
 
   proxy()->FetchContextualSuggestions(GURL(kValidFromUrl),
                                       mock_cluster_callback.ToOnceCallback());
-  service()->RunClustersCallback(ContextualSuggestionsResult());
+  service()->RunClustersCallback(ContextualSuggestionsResult(
+      kTestPeekText, std::vector<Cluster>(), PeekConditions()));
+
   EXPECT_TRUE(mock_cluster_callback.has_run);
 }
 

components/ntp_snippets/contextual/contextual_content_suggestions_service_unittest.cc

@@ -13,6 +13,7 @@
 #include "base/macros.h"
 #include "base/message_loop/message_loop.h"
 #include "base/run_loop.h"
+#include "base/strings/string_number_conversions.h"
 #include "base/test/mock_callback.h"
 #include "components/image_fetcher/core/image_fetcher_impl.h"
 #include "components/ntp_snippets/category_info.h"
@@ -82,10 +83,10 @@ class FakeContextualSuggestionsFetcher : public ContextualSuggestionsFetcher {
 // Always fetches a fake image if the given URL is valid.
 class FakeCachedImageFetcher : public CachedImageFetcher {
  public:
-  FakeCachedImageFetcher(PrefService* pref_service)
+  explicit FakeCachedImageFetcher(PrefService* pref_service)
       : CachedImageFetcher(std::unique_ptr<image_fetcher::ImageFetcher>(),
                            pref_service,
-                           nullptr){};
+                           nullptr) {}
 
   void FetchSuggestionImage(const ContentSuggestion::ID&,
                             const GURL& image_url,
@@ -202,4 +203,80 @@ TEST_F(ContextualContentSuggestionsServiceTest,
   EXPECT_EQ(mock_callback.response_peek_text, std::string());
 }
 
+TEST_F(ContextualContentSuggestionsServiceTest, ShouldCacheResults) {
+  MockClustersCallback mock_callback;
+  MockClustersCallback mock_callback2;
+  std::vector<Cluster> clusters;
+  GURL context_url("http://www.from.url");
+
+  clusters.emplace_back(ClusterBuilder("Title")
+                            .AddSuggestion(SuggestionBuilder(context_url)
+                                               .Title("Title1")
+                                               .PublisherName("from.url")
+                                               .Snippet("Summary")
+                                               .ImageId("abc")
+                                               .Build())
+                            .Build());
+  fetcher()->SetFakeResponse(clusters);
+  source()->FetchContextualSuggestionClusters(
+      context_url, mock_callback.ToOnceCallback(), base::DoNothing());
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_TRUE(mock_callback.has_run);
+
+  // The correct result should be present even though we haven't set the fake
+  // response.
+  source()->FetchContextualSuggestionClusters(
+      context_url, mock_callback2.ToOnceCallback(), base::DoNothing());
+  EXPECT_TRUE(mock_callback2.has_run);
+  ExpectResponsesMatch(
+      mock_callback2,
+      ContextualSuggestionsResult("peek text", clusters, PeekConditions()));
+}
+
+TEST_F(ContextualContentSuggestionsServiceTest, ShouldEvictOldCachedResults) {
+  std::vector<Cluster> clusters;
+  clusters.emplace_back(
+      ClusterBuilder("Title")
+          .AddSuggestion(SuggestionBuilder(GURL("http://foobar.com"))
+                             .Title("Title1")
+                             .PublisherName("from.url")
+                             .Snippet("Summary")
+                             .ImageId("abc")
+                             .Build())
+          .Build());
+
+  for (int i = 0; i < kFetchCacheCapacity + 1; i++) {
+    MockClustersCallback mock_callback;
+    GURL context_url("http://www.from.url/" + base::NumberToString(i));
+
+    fetcher()->SetFakeResponse(clusters);
+    source()->FetchContextualSuggestionClusters(
+        context_url, mock_callback.ToOnceCallback(), base::DoNothing());
+    base::RunLoop().RunUntilIdle();
+
+    ExpectResponsesMatch(
+        mock_callback,
+        ContextualSuggestionsResult("peek text", clusters, PeekConditions()));
+  }
+
+  // Urls numbered kFetchCacheCapacity through 1 should be cached still; 0
+  // should have been evicted.
+  for (int i = kFetchCacheCapacity; i > 0; i--) {
+    GURL context_url("http://www.from.url/" + base::NumberToString(i));
+    MockClustersCallback mock_callback;
+    source()->FetchContextualSuggestionClusters(
+        context_url, mock_callback.ToOnceCallback(), base::DoNothing());
+    ExpectResponsesMatch(
+        mock_callback,
+        ContextualSuggestionsResult("peek text", clusters, PeekConditions()));
+  }
+
+  GURL context_url("http://www.from.url/0");
+  MockClustersCallback mock_callback;
+  source()->FetchContextualSuggestionClusters(
+      context_url, mock_callback.ToOnceCallback(), base::DoNothing());
+  EXPECT_EQ(mock_callback.response_clusters.size(), 0u);
+}
+
 }  // namespace contextual_suggestions

components/ntp_snippets/contextual/contextual_suggestion.cc

@@ -2,6 +2,9 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include <string>
+#include <utility>
+
 #include "components/ntp_snippets/contextual/contextual_suggestion.h"
 
 namespace contextual_suggestions {
@@ -26,6 +29,9 @@ ContextualSuggestion::ContextualSuggestion(
 
 ContextualSuggestion::~ContextualSuggestion() = default;
 
+ContextualSuggestion& ContextualSuggestion::operator=(
+    const ContextualSuggestion&) = default;
+
 SuggestionBuilder::SuggestionBuilder(const GURL& url) {
   suggestion_.url = url;
   suggestion_.id = url.spec();

components/ntp_snippets/contextual/contextual_suggestion.h

@@ -5,6 +5,8 @@
 #ifndef COMPONENTS_NTP_SNIPPETS_CONTEXTUAL_CONTEXTUAL_SUGGESTION_H_
 #define COMPONENTS_NTP_SNIPPETS_CONTEXTUAL_CONTEXTUAL_SUGGESTION_H_
 
+#include <string>
+
 #include "url/gurl.h"
 
 namespace contextual_suggestions {
@@ -13,9 +15,11 @@ namespace contextual_suggestions {
 struct ContextualSuggestion {
   ContextualSuggestion();
   ContextualSuggestion(const ContextualSuggestion&);
-  ContextualSuggestion(ContextualSuggestion&&) noexcept;
+  ContextualSuggestion(ContextualSuggestion&&);
   ~ContextualSuggestion();
 
+  ContextualSuggestion& operator=(const ContextualSuggestion&);
+
   // The ID identifying the suggestion.
   std::string id;
 
@@ -48,7 +52,7 @@ struct ContextualSuggestion {
 // order has to be guessed at.
 class SuggestionBuilder {
  public:
-  SuggestionBuilder(const GURL& url);
+  explicit SuggestionBuilder(const GURL& url);
 
   SuggestionBuilder& Title(const std::string& title);
   SuggestionBuilder& PublisherName(const std::string& publisher_name);

components/ntp_snippets/contextual/contextual_suggestions_cache.cc

+// Copyright 2018 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 "components/ntp_snippets/contextual/contextual_suggestions_cache.h"
+
+namespace contextual_suggestions {
+
+ContextualSuggestionsCache::ContextualSuggestionsCache(size_t capacity)
+    : cache_(capacity) {}
+ContextualSuggestionsCache::~ContextualSuggestionsCache() = default;
+
+bool ContextualSuggestionsCache::GetSuggestionsResult(
+    const GURL& url,
+    ContextualSuggestionsResult* result) {
+  auto cache_iter = cache_.Get(url);
+  if (cache_iter == cache_.end()) {
+    return false;
+  }
+
+  *result = cache_iter->second;
+  return true;
+}
+
+void ContextualSuggestionsCache::AddSuggestionsResult(
+    const GURL& url,
+    ContextualSuggestionsResult result) {
+  cache_.Put(url, result);
+}
+
+void ContextualSuggestionsCache::Clear() {
+  cache_.Clear();
+}
+
+}  // namespace contextual_suggestions

components/ntp_snippets/contextual/contextual_suggestions_cache.h

+// Copyright 2018 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.
+
+#ifndef COMPONENTS_NTP_SNIPPETS_CONTEXTUAL_CONTEXTUAL_SUGGESTIONS_CACHE_H_
+#define COMPONENTS_NTP_SNIPPETS_CONTEXTUAL_CONTEXTUAL_SUGGESTIONS_CACHE_H_
+
+#include "base/containers/mru_cache.h"
+#include "components/ntp_snippets/contextual/contextual_suggestions_result.h"
+#include "url/gurl.h"
+
+namespace contextual_suggestions {
+
+// Wrapper for an LRU cache of ContextualSuggestionResult objects, keyed by
+// context URL.
+class ContextualSuggestionsCache {
+ public:
+  explicit ContextualSuggestionsCache(size_t capacity);
+  ~ContextualSuggestionsCache();
+
+  // Attempts to find a result for |url| in this cache, returning true and
+  // putting the result into |result| if successful.
+  bool GetSuggestionsResult(const GURL& url,
+                            ContextualSuggestionsResult* result);
+  // Adds |result| to this cache for the key |url|, overwriting any previous
+  // value associated with |url| and potentially evicting the oldest item in the
+  // cache.
+  void AddSuggestionsResult(const GURL& url,
+                            ContextualSuggestionsResult result);
+  // Removes all items from the cache.
+  void Clear();
+
+ private:
+  base::MRUCache<GURL, ContextualSuggestionsResult> cache_;
+};
+
+}  // namespace contextual_suggestions
+
+#endif  // COMPONENTS_NTP_SNIPPETS_CONTEXTUAL_CONTEXTUAL_SUGGESTIONS_CACHE_H_

components/ntp_snippets/contextual/contextual_suggestions_result.cc

@@ -16,16 +16,18 @@ PeekConditions::PeekConditions()
 
 Cluster::Cluster() = default;
 
+Cluster::Cluster(const Cluster& other) = default;
+
 // MSVC doesn't support defaulted move constructors, so we have to define it
 // ourselves.
 Cluster::Cluster(Cluster&& other) noexcept
     : title(std::move(other.title)),
       suggestions(std::move(other.suggestions)) {}
 
-Cluster::Cluster(const Cluster&) = default;
-
 Cluster::~Cluster() = default;
 
+Cluster& Cluster::operator=(const Cluster&) = default;
+
 ClusterBuilder::ClusterBuilder(const std::string& title) {
   cluster_.title = title;
 }
@@ -79,4 +81,7 @@ ContextualSuggestionsResult::~ContextualSuggestionsResult() = default;
 ContextualSuggestionsResult& ContextualSuggestionsResult::operator=(
     ContextualSuggestionsResult&& other) = default;
 
+ContextualSuggestionsResult& ContextualSuggestionsResult::operator=(
+    const ContextualSuggestionsResult& other) = default;
+
 }  // namespace contextual_suggestions

components/ntp_snippets/contextual/contextual_suggestions_result.h

@@ -7,6 +7,10 @@
 
 #include "components/ntp_snippets/contextual/contextual_suggestion.h"
 
+#include <string>
+#include <utility>
+#include <vector>
+
 namespace contextual_suggestions {
 
 // Encapsulates conditions under which to show or "peek" the contextual
@@ -36,6 +40,8 @@ struct Cluster {
   Cluster(Cluster&&) noexcept;
   ~Cluster();
 
+  Cluster& operator=(const Cluster&);
+
   std::string title;
   std::vector<ContextualSuggestion> suggestions;
 };
@@ -43,7 +49,7 @@ struct Cluster {
 // Allows concise construction of a cluster.
 class ClusterBuilder {
  public:
-  ClusterBuilder(const std::string& title);
+  explicit ClusterBuilder(const std::string& title);
 
   // Allow copying for ease of validation when testing.
   ClusterBuilder(const ClusterBuilder& other);
@@ -66,6 +72,7 @@ struct ContextualSuggestionsResult {
   ~ContextualSuggestionsResult();
 
   ContextualSuggestionsResult& operator=(ContextualSuggestionsResult&&);
+  ContextualSuggestionsResult& operator=(const ContextualSuggestionsResult&);
 
   std::vector<Cluster> clusters;
   std::string peek_text;