[Feed] Break offline pages ties by namespace first.

Bug: 888767
Change-Id: Iaac4393ba4609095b58d00dbcfb1234e20296a0e
Reviewed-on: https://chromium-review.googlesource.com/c/1279045
Commit-Queue: Sky Malice <skym@chromium.org>
Reviewed-by: Filip Gorski <fgorski@chromium.org>
Cr-Commit-Position: refs/heads/master@{#600089}

components/feed/content/feed_offline_host.cc

@@ -11,6 +11,7 @@
 #include "base/metrics/histogram_macros.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "components/feed/core/feed_scheduler_host.h"
+#include "components/offline_pages/core/client_namespace_constants.h"
 #include "components/offline_pages/core/prefetch/prefetch_service.h"
 #include "url/gurl.h"
 
@@ -53,12 +54,26 @@ class CallbackAggregator : public base::RefCounted<CallbackAggregator> {
   void OnGetPages(std::string feed_url,
                   const std::vector<OfflinePageItem>& pages) {
     if (!pages.empty()) {
-      OfflinePageItem newest =
+      OfflinePageItem best =
           *std::max_element(pages.begin(), pages.end(), [](auto lhs, auto rhs) {
-            return lhs.creation_time < rhs.creation_time;
+            // Prefer prefetched articles over any other. They are typically of
+            // higher quality.
+            bool leftIsPrefetch = lhs.client_id.name_space ==
+                                  offline_pages::kSuggestedArticlesNamespace;
+            bool rightIsPrefetch = rhs.client_id.name_space ==
+                                   offline_pages::kSuggestedArticlesNamespace;
+            if (leftIsPrefetch != rightIsPrefetch) {
+              // Only one is prefetch, if that is |rhs|, then they're in the
+              // correct order.
+              return rightIsPrefetch;
+            } else {
+              // Newer articles are also better, but not as important as being
+              // prefetched.
+              return lhs.creation_time < rhs.creation_time;
+            }
           });
       urls_.push_back(feed_url);
-      on_each_result_.Run(std::move(feed_url), newest.offline_id);
+      on_each_result_.Run(feed_url, best.offline_id);
     }
   }
 

components/feed/content/feed_offline_host_unittest.cc

@@ -15,6 +15,7 @@
 #include "base/test/scoped_task_environment.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "components/feed/core/content_metadata.h"
+#include "components/offline_pages/core/client_namespace_constants.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"
@@ -52,12 +53,14 @@ class TestOfflinePageModel : public StubOfflinePageModel {
   void AddOfflinedPage(const std::string& url,
                        const std::string& original_url,
                        int64_t offline_id,
-                       base::Time creation_time) {
+                       base::Time creation_time,
+                       std::string name_space) {
     OfflinePageItem item;
     item.url = GURL(url);
     item.original_url = GURL(original_url);
     item.offline_id = offline_id;
     item.creation_time = creation_time;
+    item.client_id = offline_pages::ClientId(name_space, "");
     url_to_offline_page_item_.emplace(url, item);
     if (!original_url.empty()) {
       url_to_offline_page_item_.emplace(original_url, item);
@@ -65,7 +68,7 @@ class TestOfflinePageModel : public StubOfflinePageModel {
   }
 
   void AddOfflinedPage(const std::string& url, int64_t offline_id) {
-    AddOfflinedPage(url, "", offline_id, base::Time());
+    AddOfflinedPage(url, "", offline_id, base::Time(), "");
   }
 
   MOCK_METHOD1(AddObserver, void(Observer*));
@@ -233,7 +236,7 @@ TEST_F(FeedOfflineHostTest, GetOfflineStatusHit) {
 }
 
 TEST_F(FeedOfflineHostTest, GetOfflineIdOriginalUrl) {
-  offline_page_model()->AddOfflinedPage(kUrl1, kUrl2, 4, base::Time());
+  offline_page_model()->AddOfflinedPage(kUrl1, kUrl2, 4, base::Time(), "");
 
   std::vector<std::string> actual;
   host()->GetOfflineStatus({kUrl2}, base::BindOnce(&CopyStatus, &actual));
@@ -246,7 +249,7 @@ TEST_F(FeedOfflineHostTest, GetOfflineIdOriginalUrl) {
 }
 
 TEST_F(FeedOfflineHostTest, GetOfflineIdRequestUrl) {
-  offline_page_model()->AddOfflinedPage(kUrl2, kUrl1, 4, base::Time());
+  offline_page_model()->AddOfflinedPage(kUrl2, kUrl1, 4, base::Time(), "");
 
   std::vector<std::string> actual;
   host()->GetOfflineStatus({kUrl2}, base::BindOnce(&CopyStatus, &actual));
@@ -259,9 +262,9 @@ TEST_F(FeedOfflineHostTest, GetOfflineIdRequestUrl) {
 }
 
 TEST_F(FeedOfflineHostTest, GetOfflineIdNewer) {
-  offline_page_model()->AddOfflinedPage(kUrl1, "", 4, base::Time());
+  offline_page_model()->AddOfflinedPage(kUrl1, "", 4, base::Time(), "");
   offline_page_model()->AddOfflinedPage(
-      kUrl1, "", 5, base::Time() + base::TimeDelta::FromHours(1));
+      kUrl1, "", 5, base::Time() + base::TimeDelta::FromHours(1), "");
 
   std::vector<std::string> actual;
   host()->GetOfflineStatus({kUrl1}, base::BindOnce(&CopyStatus, &actual));
@@ -272,6 +275,23 @@ TEST_F(FeedOfflineHostTest, GetOfflineIdNewer) {
   EXPECT_EQ(host()->GetOfflineId(kUrl1).value(), 5);
 }
 
+TEST_F(FeedOfflineHostTest, GetOfflineIdNamespace) {
+  // Even though id of 5 is newer, id of 4 will be chosen because it has the
+  // preferred namespace.
+  offline_page_model()->AddOfflinedPage(
+      kUrl1, "", 4, base::Time(), offline_pages::kSuggestedArticlesNamespace);
+  offline_page_model()->AddOfflinedPage(
+      kUrl1, "", 5, base::Time() + base::TimeDelta::FromHours(1), "");
+
+  std::vector<std::string> actual;
+  host()->GetOfflineStatus({kUrl1}, base::BindOnce(&CopyStatus, &actual));
+  RunUntilIdle();
+
+  EXPECT_EQ(1U, actual.size());
+  EXPECT_EQ(kUrl1, actual[0]);
+  EXPECT_EQ(host()->GetOfflineId(kUrl1).value(), 4);
+}
+
 TEST_F(FeedOfflineHostTest, GetCurrentArticleSuggestions) {
   std::vector<PrefetchSuggestion> actual;
   host()->GetCurrentArticleSuggestions(