[Feed] Implement native logic for OfflineIndicatorApi::getOfflineStatus().

This change feeds offline status information to the Feed component so that
it can properly display the offline badges on articles that are downloaded.
The call to actually open Feed articles with custom load params is not
done in this change, but the cache FeedOfflineHost holds is preparing for
this eventual use case.

Bug: 866125
Change-Id: Ie28cfe0913d9d93f197d5aba1bff181369d3bf39
Reviewed-on: https://chromium-review.googlesource.com/1197268
Commit-Queue: Sky Malice <skym@chromium.org>
Reviewed-by: Steven Holte <holte@chromium.org>
Reviewed-by: Filip Gorski <fgorski@chromium.org>
Reviewed-by: Gang Wu <gangwu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#589210}

chrome/browser/android/feed/feed_offline_bridge.cc

@@ -32,7 +32,7 @@ namespace feed {
 namespace {
 
 void OnGetOfflineStatus(ScopedJavaGlobalRef<jobject> callback,
-                        const std::vector<std::string>& urls) {
+                        std::vector<std::string> urls) {
   JNIEnv* env = base::android::AttachCurrentThread();
   ScopedJavaLocalRef<jobjectArray> j_urls =
       base::android::ToJavaArrayOfStrings(env, urls);
@@ -82,7 +82,7 @@ void FeedOfflineBridge::GetOfflineStatus(JNIEnv* env,
   base::android::AppendJavaStringArrayToStringVector(env, j_urls.obj(), &urls);
   ScopedJavaGlobalRef<jobject> callback(j_callback);
   offline_host_->GetOfflineStatus(
-      urls, base::BindOnce(&OnGetOfflineStatus, callback));
+      std::move(urls), base::BindOnce(&OnGetOfflineStatus, callback));
 }
 
 void FeedOfflineBridge::OnContentRemoved(

components/feed/content/BUILD.gn

@@ -36,6 +36,7 @@ source_set("content_unit_tests") {
     ":feed_content",
     "//base",
     "//base/test:test_support",
+    "//components/offline_pages/core",
     "//components/offline_pages/core:test_support",
     "//components/offline_pages/core/prefetch:test_support",
     "//testing/gtest:gtest",

components/feed/content/feed_offline_host.cc

@@ -4,10 +4,79 @@
 
 #include "components/feed/content/feed_offline_host.h"
 
+#include <utility>
+
+#include "base/bind.h"
+#include "base/hash.h"
+#include "base/metrics/histogram_macros.h"
+#include "components/feed/core/feed_scheduler_host.h"
+#include "components/offline_pages/core/prefetch/prefetch_service.h"
 #include "url/gurl.h"
 
 namespace feed {
 
+namespace {
+
+// |url| is always set. Sometimes |original_url| is set. If |original_url| is
+// set it is returned by this method, otherwise fall back to |url|.
+const GURL& PreferOriginal(const offline_pages::OfflinePageItem& item) {
+  return item.original_url.is_empty() ? item.url : item.original_url;
+}
+
+// Aggregates multiple callbacks from OfflinePageModel, storing the offline url.
+// When all callbacks have been invoked, tracked by ref counting, then
+// |on_completeion_| is finally invoked, sending all results together.
+class CallbackAggregator : public base::RefCounted<CallbackAggregator> {
+ public:
+  using ReportStatusCallback =
+      base::OnceCallback<void(std::vector<std::string>)>;
+  using CacheIdCallback =
+      base::RepeatingCallback<void(const std::string&, int64_t)>;
+
+  CallbackAggregator(ReportStatusCallback on_completion,
+                     CacheIdCallback on_each_result)
+      : on_completion_(std::move(on_completion)),
+        on_each_result_(std::move(on_each_result)),
+        start_time_(base::Time::Now()) {}
+
+  void OnGetPages(const std::vector<offline_pages::OfflinePageItem>& pages) {
+    if (!pages.empty()) {
+      offline_pages::OfflinePageItem newest =
+          *std::max_element(pages.begin(), pages.end(), [](auto lhs, auto rhs) {
+            return lhs.creation_time < rhs.creation_time;
+          });
+      const std::string& url = PreferOriginal(newest).spec();
+      urls_.push_back(url);
+      on_each_result_.Run(url, newest.offline_id);
+    }
+  }
+
+ private:
+  friend class base::RefCounted<CallbackAggregator>;
+
+  ~CallbackAggregator() {
+    base::TimeDelta duration = base::Time::Now() - start_time_;
+    UMA_HISTOGRAM_TIMES("Feed.Offline.GetStatusDuration", duration);
+    std::move(on_completion_).Run(std::move(urls_));
+  }
+
+  // To be called once all callbacks are run or destroyed.
+  ReportStatusCallback on_completion_;
+
+  // The urls of the offlined pages seen so far. Ultimately will be given to
+  // |on_completeion_|.
+  std::vector<std::string> urls_;
+
+  // Is not run if there are no results for a given url.
+  CacheIdCallback on_each_result_;
+
+  // The time the aggregator was created, before any requests were sent to the
+  // OfflinePageModel.
+  base::Time start_time_;
+};
+
+}  //  namespace
+
 FeedOfflineHost::FeedOfflineHost(
     offline_pages::OfflinePageModel* offline_page_model,
     offline_pages::PrefetchService* prefetch_service,
@@ -26,14 +95,27 @@ FeedOfflineHost::FeedOfflineHost(
 
 FeedOfflineHost::~FeedOfflineHost() = default;
 
-base::Optional<int64_t> FeedOfflineHost::GetOfflineId(std::string url) {
-  return {};
+base::Optional<int64_t> FeedOfflineHost::GetOfflineId(const std::string& url) {
+  auto iter = url_hash_to_id_.find(base::Hash(url));
+  return iter == url_hash_to_id_.end() ? base::Optional<int64_t>()
+                                       : base::Optional<int64_t>(iter->second);
 }
 
 void FeedOfflineHost::GetOfflineStatus(
     std::vector<std::string> urls,
-    base::OnceCallback<void(const std::vector<std::string>&)> callback) {
-  // TODO(skym): Call OfflinePageModel::GetPagesByURL() for each url.
+    base::OnceCallback<void(std::vector<std::string>)> callback) {
+  UMA_HISTOGRAM_EXACT_LINEAR("Feed.Offline.GetStatusCount", urls.size(), 50);
+
+  scoped_refptr<CallbackAggregator> aggregator =
+      base::MakeRefCounted<CallbackAggregator>(
+          std::move(callback),
+          base::BindRepeating(&FeedOfflineHost::CacheOfflinePageAndId,
+                              weak_ptr_factory_.GetWeakPtr()));
+
+  for (std::string url : urls) {
+    offline_page_model_->GetPagesByURL(
+        GURL(url), base::BindOnce(&CallbackAggregator::OnGetPages, aggregator));
+  }
 }
 
 void FeedOfflineHost::OnContentRemoved(std::vector<std::string> urls) {
@@ -78,4 +160,9 @@ void FeedOfflineHost::OfflinePageDeleted(
   // TODO(skym): Call into bridge callback.
 }
 
+void FeedOfflineHost::CacheOfflinePageAndId(const std::string& url,
+                                            int64_t id) {
+  url_hash_to_id_[base::Hash(url)] = id;
+}
+
 }  // namespace feed

components/feed/content/feed_offline_host.h

@@ -9,6 +9,7 @@
 #include <vector>
 
 #include "base/callback.h"
+#include "base/containers/flat_map.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "base/optional.h"
@@ -43,13 +44,13 @@ class FeedOfflineHost : public offline_pages::SuggestionsProvider,
   // receive a false negative. Additionally, since the host tracks pages by
   // hashing, there's also a small chance that the host erroneously returns an
   // id for a page that is not offlined.
-  base::Optional<int64_t> GetOfflineId(std::string url);
+  base::Optional<int64_t> GetOfflineId(const std::string& url);
 
   // Asynchronously fetches offline status for the given URLs. Any pages that
   // are currently offlined will be remembered by the FeedOfflineHost.
   void GetOfflineStatus(
       std::vector<std::string> urls,
-      base::OnceCallback<void(const std::vector<std::string>&)> callback);
+      base::OnceCallback<void(std::vector<std::string>)> callback);
 
   // Should be called from Feed any time the user manually removes articles or
   // groupings of articles. Propagates the signal to Prefetch.
@@ -80,6 +81,10 @@ class FeedOfflineHost : public offline_pages::SuggestionsProvider,
       override;
 
  private:
+  // Stores the given record in |url_hash_to_id_|. If there's a conflict, the
+  // new id will overwrite the old value.
+  void CacheOfflinePageAndId(const std::string& url, int64_t id);
+
   // The following objects all outlive us, so it is safe to hold raw pointers to
   // them. This is guaranteed by the FeedHostServiceFactory.
   offline_pages::OfflinePageModel* offline_page_model_;
@@ -88,6 +93,12 @@ class FeedOfflineHost : public offline_pages::SuggestionsProvider,
   base::RepeatingClosure on_suggestion_consumed_;
   base::RepeatingClosure on_suggestions_shown_;
 
+  // Only offlined pages that have passed through the host are stored. If there
+  // are ever no listeners to the offline host logic and OnNoListeners() is
+  // called this map is cleared. The key is the hash of the url, and the value
+  // is the offline id for the given page.
+  base::flat_map<uint32_t, int64_t> url_hash_to_id_;
+
   base::WeakPtrFactory<FeedOfflineHost> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(FeedOfflineHost);

components/feed/content/feed_offline_host_unittest.cc

@@ -4,21 +4,86 @@
 
 #include "components/feed/content/feed_offline_host.h"
 
+#include <algorithm>
+#include <map>
 #include <memory>
+#include <utility>
 
 #include "base/bind.h"
+#include "base/task/post_task.h"
+#include "base/test/scoped_task_environment.h"
+#include "base/threading/thread_task_runner_handle.h"
+#include "components/offline_pages/core/offline_page_item.h"
+#include "components/offline_pages/core/offline_page_types.h"
 #include "components/offline_pages/core/prefetch/stub_prefetch_service.h"
 #include "components/offline_pages/core/stub_offline_page_model.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace feed {
 
+namespace {
+
+const char kUrl1[] = "https://www.one.com/";
+const char kUrl2[] = "https://www.two.com/";
+const char kUrl3[] = "https://www.three.com/";
+
+class TestOfflinePageModel : public offline_pages::StubOfflinePageModel {
+ public:
+  void AddOfflinedPage(const std::string& url,
+                       const std::string& original_url,
+                       int64_t offline_id,
+                       base::Time creation_time) {
+    offline_pages::OfflinePageItem item;
+    item.url = GURL(url);
+    item.original_url = GURL(original_url);
+    item.offline_id = offline_id;
+    item.creation_time = creation_time;
+    url_to_offline_page_item_.emplace(url, item);
+    if (!original_url.empty()) {
+      url_to_offline_page_item_.emplace(original_url, item);
+    }
+  }
+
+  void AddOfflinedPage(const std::string& url, int64_t offline_id) {
+    AddOfflinedPage(url, "", offline_id, base::Time());
+  }
+
+ private:
+  void GetPagesByURL(
+      const GURL& url,
+      offline_pages::MultipleOfflinePageItemCallback callback) override {
+    auto iter = url_to_offline_page_item_.equal_range(url.spec());
+    std::vector<offline_pages::OfflinePageItem> ret;
+    ret.resize(std::distance(iter.first, iter.second));
+    std::transform(iter.first, iter.second, ret.begin(),
+                   [](auto pair) { return pair.second; });
+
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE, base::BindOnce(std::move(callback), std::move(ret)));
+  }
+
+  // Maps URLs to offline_pages::OfflinePageItem. Items with both |urls| and
+  // |original_url| will be inserted at both locations in the multimap.
+  std::multimap<std::string, offline_pages::OfflinePageItem>
+      url_to_offline_page_item_;
+};
+
+void CopyResults(std::vector<std::string>* actual,
+                 std::vector<std::string> result) {
+  *actual = result;
+}
+
+}  // namespace
+
 class FeedOfflineHostTest : public ::testing::Test {
  public:
+  TestOfflinePageModel* offline_page_model() { return &offline_page_model_; }
   FeedOfflineHost* host() { return host_.get(); }
   int get_suggestion_consumed_count() { return suggestion_consumed_count_; }
   int get_suggestions_shown_count() { return suggestions_shown_count_; }
 
+  void RunUntilIdle() { scoped_task_environment_.RunUntilIdle(); }
+
  protected:
   FeedOfflineHostTest() {
     host_ = std::make_unique<FeedOfflineHost>(
@@ -32,7 +97,8 @@ class FeedOfflineHostTest : public ::testing::Test {
   void OnSuggestionConsumed() { ++suggestion_consumed_count_; }
   void OnSuggestionsShown() { ++suggestions_shown_count_; }
 
-  offline_pages::StubOfflinePageModel offline_page_model_;
+  base::test::ScopedTaskEnvironment scoped_task_environment_;
+  TestOfflinePageModel offline_page_model_;
   offline_pages::StubPrefetchService prefetch_service_;
   std::unique_ptr<FeedOfflineHost> host_;
   int suggestion_consumed_count_ = 0;
@@ -50,11 +116,78 @@ TEST_F(FeedOfflineHostTest, ReportArticleListViewed) {
 
 TEST_F(FeedOfflineHostTest, OnSuggestionsShown) {
   EXPECT_EQ(0, get_suggestions_shown_count());
-  host()->ReportArticleViewed(GURL("https://www.one.com"));
+  host()->ReportArticleViewed(GURL(kUrl1));
   EXPECT_EQ(1, get_suggestions_shown_count());
-  host()->ReportArticleViewed(GURL("https://www.one.com"));
+  host()->ReportArticleViewed(GURL(kUrl1));
   EXPECT_EQ(2, get_suggestions_shown_count());
   EXPECT_EQ(0, get_suggestion_consumed_count());
 }
 
+TEST_F(FeedOfflineHostTest, GetOfflineStatusEmpty) {
+  std::vector<std::string> actual;
+  host()->GetOfflineStatus({}, base::BindOnce(&CopyResults, &actual));
+  RunUntilIdle();
+
+  EXPECT_EQ(0U, actual.size());
+}
+
+TEST_F(FeedOfflineHostTest, GetOfflineStatusMiss) {
+  offline_page_model()->AddOfflinedPage(kUrl1, 4);
+
+  std::vector<std::string> actual;
+  host()->GetOfflineStatus({kUrl2}, base::BindOnce(&CopyResults, &actual));
+  RunUntilIdle();
+
+  EXPECT_EQ(0U, actual.size());
+  EXPECT_FALSE(host()->GetOfflineId(kUrl1).has_value());
+  EXPECT_FALSE(host()->GetOfflineId(kUrl2).has_value());
+}
+
+TEST_F(FeedOfflineHostTest, GetOfflineStatusHit) {
+  offline_page_model()->AddOfflinedPage(kUrl1, 4);
+  offline_page_model()->AddOfflinedPage(kUrl2, 5);
+  offline_page_model()->AddOfflinedPage(kUrl3, 6);
+
+  std::vector<std::string> actual;
+  host()->GetOfflineStatus({kUrl1, kUrl2},
+                           base::BindOnce(&CopyResults, &actual));
+
+  EXPECT_EQ(0U, actual.size());
+  RunUntilIdle();
+
+  EXPECT_EQ(2U, actual.size());
+  EXPECT_TRUE(actual[0] == kUrl1 || actual[1] == kUrl1);
+  EXPECT_TRUE(actual[0] == kUrl2 || actual[1] == kUrl2);
+  EXPECT_EQ(host()->GetOfflineId(kUrl1).value(), 4);
+  EXPECT_EQ(host()->GetOfflineId(kUrl2).value(), 5);
+  EXPECT_FALSE(host()->GetOfflineId(kUrl3).has_value());
+}
+
+TEST_F(FeedOfflineHostTest, GetOfflineIdOriginalUrl) {
+  offline_page_model()->AddOfflinedPage(kUrl1, kUrl2, 4, base::Time());
+
+  std::vector<std::string> actual;
+  host()->GetOfflineStatus({kUrl2}, base::BindOnce(&CopyResults, &actual));
+  RunUntilIdle();
+
+  EXPECT_EQ(1U, actual.size());
+  EXPECT_EQ(kUrl2, actual[0]);
+  EXPECT_FALSE(host()->GetOfflineId(kUrl1).has_value());
+  EXPECT_EQ(host()->GetOfflineId(kUrl2).value(), 4);
+}
+
+TEST_F(FeedOfflineHostTest, GetOfflineIdNewer) {
+  offline_page_model()->AddOfflinedPage(kUrl1, "", 4, base::Time());
+  offline_page_model()->AddOfflinedPage(
+      kUrl1, "", 5, base::Time() + base::TimeDelta::FromHours(1));
+
+  std::vector<std::string> actual;
+  host()->GetOfflineStatus({kUrl1}, base::BindOnce(&CopyResults, &actual));
+  RunUntilIdle();
+
+  EXPECT_EQ(1U, actual.size());
+  EXPECT_EQ(kUrl1, actual[0]);
+  EXPECT_EQ(host()->GetOfflineId(kUrl1).value(), 5);
+}
+
 }  // namespace feed

tools/metrics/histograms/histograms.xml

@@ -31609,6 +31609,23 @@ uploading your change for review.
   </summary>
 </histogram>
 
+<histogram name="Feed.Offline.GetStatusCount" units="count"
+    expires_after="2019-08-30">
+  <owner>skym@chromium.org</owner>
+  <summary>
+    The number of urls that have offline status requested per call.
+  </summary>
+</histogram>
+
+<histogram name="Feed.Offline.GetStatusDuration" units="ms"
+    expires_after="2019-08-30">
+  <owner>skym@chromium.org</owner>
+  <summary>
+    The number of milliseconds to round trip offline status for a set of URLs
+    from Offline Pages component.
+  </summary>
+</histogram>
+
 <histogram name="Feed.Scheduler.RefreshTrigger" enum="RefreshTrigger"
     expires_after="2019-07-11">
   <owner>skym@chromium.org</owner>