Show suggested articles tab when opening downloads home if any exist

This change replaces the previous, partial implementation of the
detection of available prefetched articles when choosing how to present
the Downloads Home when it is being launched from the dino page. The
logic was moved from the mojom service implementation to the renderer
side helper for available content and now supports both the summary and
list presentation modes.

Bug: 852872
Change-Id: I82652e7a19f5a864e8cd56882eb06fb44593a994
Reviewed-on: https://chromium-review.googlesource.com/c/1235373
Commit-Queue: Carlos Knippschild <carlosk@chromium.org>
Reviewed-by: Ken Buchanan <kenrb@chromium.org>
Reviewed-by: Cathy Li <chili@chromium.org>
Reviewed-by: Min Qin <qinmin@chromium.org>
Reviewed-by: Dan H <harringtond@google.com>
Cr-Commit-Position: refs/heads/master@{#597619}

chrome/browser/android/download/available_offline_content_provider.cc

@@ -4,6 +4,9 @@
 
 #include "chrome/browser/android/download/available_offline_content_provider.h"
 
+#include <memory>
+#include <utility>
+
 #include "base/base64.h"
 #include "base/numerics/safe_conversions.h"
 #include "base/strings/strcat.h"
@@ -226,9 +229,10 @@ void AvailableOfflineContentProvider::LaunchItem(
       offline_items_collection::ContentId(name_space, item_id));
 }
 
-void AvailableOfflineContentProvider::LaunchDownloadsPage() {
+void AvailableOfflineContentProvider::LaunchDownloadsPage(
+    bool open_prefetched_articles_tab) {
   DownloadManagerService::GetInstance()->ShowDownloadManager(
-      has_prefetched_content_);
+      open_prefetched_articles_tab);
 }
 
 void AvailableOfflineContentProvider::Create(
@@ -268,7 +272,6 @@ void AvailableOfflineContentProvider::SummarizeFinalize(
         break;
     }
   }
-  has_prefetched_content_ = summary->has_prefetched_page;
 
   // If the number of interesting items is lower then the minimum required then
   // reset all summary data so avoid presenting the card.
@@ -289,12 +292,12 @@ void AvailableOfflineContentProvider::ListFinalize(
   // If the number of interesting items is lower then the minimum don't show any
   // suggestions. Otherwise trim it down to the number of expected items.
   size_t copied_count = end - selected.begin();
-  if (copied_count == kMinInterestingItemCount &&
-      ContentType(selected[kMinInterestingItemCount - 1]) !=
-          AvailableContentType::kUninteresting) {
-    selected.resize(kMaxListItemsToReturn);
-  } else {
+  DCHECK(copied_count <= kMinInterestingItemCount);
+  if (copied_count < kMinInterestingItemCount ||
+      ContentType(selected.back()) == AvailableContentType::kUninteresting) {
     selected.clear();
+  } else {
+    selected.resize(kMaxListItemsToReturn);
   }
 
   std::vector<offline_items_collection::ContentId> selected_ids;

chrome/browser/android/download/available_offline_content_provider.h

@@ -5,6 +5,9 @@
 #ifndef CHROME_BROWSER_ANDROID_DOWNLOAD_AVAILABLE_OFFLINE_CONTENT_PROVIDER_H_
 #define CHROME_BROWSER_ANDROID_DOWNLOAD_AVAILABLE_OFFLINE_CONTENT_PROVIDER_H_
 
+#include <string>
+#include <vector>
+
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "chrome/common/available_offline_content.mojom.h"
@@ -34,7 +37,7 @@ class AvailableOfflineContentProvider
   void Summarize(SummarizeCallback) override;
   void LaunchItem(const std::string& item_id,
                   const std::string& name_space) override;
-  void LaunchDownloadsPage() override;
+  void LaunchDownloadsPage(bool open_prefetched_articles_tab) override;
 
   static void Create(
       content::BrowserContext* browser_context,
@@ -52,11 +55,6 @@ class AvailableOfflineContentProvider
 
   content::BrowserContext* browser_context_;
 
-  // Records if the last content fetch indicated that prefetched articles are
-  // available or not.
-  // TODO(carlosk): Directly check the existence of prefetched articles.
-  bool has_prefetched_content_ = false;
-
   base::WeakPtrFactory<AvailableOfflineContentProvider> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(AvailableOfflineContentProvider);

chrome/browser/android/download/available_offline_content_provider_unittest.cc

@@ -53,8 +53,6 @@ OfflineItem OfflinePageItem() {
   item.filter = offline_items_collection::FILTER_PAGE;
   item.id.id = "NonSuggestedOfflinePage";
   item.id.name_space = kProviderNamespace;
-  // Configuring this item as being read to make sure that's not taken into
-  // account for filtering and prioritization.
   item.last_accessed_time = base::Time::Now();
   return item;
 }
@@ -72,8 +70,6 @@ OfflineItem SuggestedOfflinePageItem() {
   // even if the test takes 1 hour to run.
   item.creation_time =
       base::Time::Now() - base::TimeDelta::FromMinutes(60 * 3.5);
-  // Configuring this item as being read to make sure that's not taken into
-  // account for filtering and prioritization.
   item.last_accessed_time = base::Time::Now();
   return item;
 }
@@ -129,6 +125,7 @@ OfflineItemVisuals TestThumbnail() {
 class AvailableOfflineContentTest : public testing::Test {
  protected:
   void SetUp() override {
+    scoped_feature_list_->InitAndEnableFeature(features::kNewNetErrorPageUI);
     // To control the items in the aggregator, we create it and register a
     // single MockOfflineContentProvider.
     aggregator_ = static_cast<OfflineContentAggregator*>(
@@ -156,14 +153,14 @@ class AvailableOfflineContentTest : public testing::Test {
 
   content::TestBrowserThreadBundle thread_bundle_;
   TestingProfile profile_;
-  base::test::ScopedFeatureList scoped_feature_list_;
+  std::unique_ptr<base::test::ScopedFeatureList> scoped_feature_list_ =
+      std::make_unique<base::test::ScopedFeatureList>();
   OfflineContentAggregator* aggregator_;
   offline_items_collection::MockOfflineContentProvider content_provider_;
   AvailableOfflineContentProvider provider_{&profile_};
 };
 
 TEST_F(AvailableOfflineContentTest, NoContent) {
-  scoped_feature_list_.InitAndEnableFeature(features::kNewNetErrorPageUI);
   std::vector<chrome::mojom::AvailableOfflineContentPtr> suggestions =
       ListAndWait();
   chrome::mojom::AvailableOfflineContentSummaryPtr summary = SummarizeAndWait();
@@ -177,7 +174,6 @@ TEST_F(AvailableOfflineContentTest, NoContent) {
 }
 
 TEST_F(AvailableOfflineContentTest, TooFewInterestingItems) {
-  scoped_feature_list_.InitAndEnableFeature(features::kNewNetErrorPageUI);
   // Adds items so that we're one-ff of reaching the minimum required count so
   // that any extra item considered interesting would effect the results.
   content_provider_.SetItems({UninterestingImageItem(), OfflinePageItem(),
@@ -202,7 +198,6 @@ TEST_F(AvailableOfflineContentTest, TooFewInterestingItems) {
 }
 
 TEST_F(AvailableOfflineContentTest, FourInterestingItems) {
-  scoped_feature_list_.InitAndEnableFeature(features::kNewNetErrorPageUI);
   // We need at least 4 interesting items for anything to show up at all.
   content_provider_.SetItems({UninterestingImageItem(), VideoItem(),
                               SuggestedOfflinePageItem(), AudioItem(),
@@ -251,7 +246,8 @@ TEST_F(AvailableOfflineContentTest, FourInterestingItems) {
 }
 
 TEST_F(AvailableOfflineContentTest, NotEnabled) {
-  scoped_feature_list_.InitAndDisableFeature(features::kNewNetErrorPageUI);
+  scoped_feature_list_ = std::make_unique<base::test::ScopedFeatureList>();
+  scoped_feature_list_->InitAndDisableFeature(features::kNewNetErrorPageUI);
   content_provider_.SetItems({SuggestedOfflinePageItem()});
 
   std::vector<chrome::mojom::AvailableOfflineContentPtr> suggestions =

chrome/common/available_offline_content.mojom

@@ -68,5 +68,5 @@ interface AvailableOfflineContentProvider {
   // Opens one of the items returned by List().
   LaunchItem(string item_id, string name_space);
   // Opens the downloads page to view all offline content.
-  LaunchDownloadsPage();
+  LaunchDownloadsPage(bool open_prefetched_articles_tab);
 };

chrome/renderer/net/available_offline_content_helper.cc

@@ -19,6 +19,7 @@
 namespace {
 
 using chrome::mojom::AvailableOfflineContentPtr;
+using chrome::mojom::AvailableContentType;
 
 base::Value AvailableContentToValue(const AvailableOfflineContentPtr& content) {
   base::Value value(base::Value::Type::DICTIONARY);
@@ -111,13 +112,12 @@ void AvailableOfflineContentHelper::LaunchItem(const std::string& id,
   if (!BindProvider())
     return;
 
-  provider_->LaunchItem(id, name_space);
-
   for (const AvailableOfflineContentPtr& item : fetched_content_) {
     if (item->id == id && item->name_space == name_space) {
       UMA_HISTOGRAM_ENUMERATION("Net.ErrorPageCounts.SuggestionClicked",
                                 item->content_type);
       RecordEvent(error_page::NETWORK_ERROR_PAGE_OFFLINE_SUGGESTION_CLICKED);
+      provider_->LaunchItem(id, name_space);
       return;
     }
   }
@@ -128,16 +128,22 @@ void AvailableOfflineContentHelper::LaunchDownloadsPage() {
   if (!BindProvider())
     return;
   RecordEvent(error_page::NETWORK_ERROR_PAGE_OFFLINE_DOWNLOADS_PAGE_CLICKED);
-  provider_->LaunchDownloadsPage();
+  provider_->LaunchDownloadsPage(has_prefetched_content_);
 }
 
 void AvailableOfflineContentHelper::AvailableContentReceived(
     base::OnceCallback<void(const std::string& offline_content_json)> callback,
     std::vector<AvailableOfflineContentPtr> content) {
+  has_prefetched_content_ = false;
   fetched_content_ = std::move(content);
   std::string json;
 
   if (!fetched_content_.empty()) {
+    // As prefetched content has the highest priority if at least one piece is
+    // available it will be the at the first position on the list.
+    has_prefetched_content_ = fetched_content_.front()->content_type ==
+                              AvailableContentType::kPrefetchedPage;
+
     RecordSuggestionPresented(fetched_content_);
     RecordEvent(error_page::NETWORK_ERROR_PAGE_OFFLINE_SUGGESTIONS_SHOWN);
     base::JSONWriter::Write(AvailableContentListToValue(fetched_content_),
@@ -153,9 +159,12 @@ void AvailableOfflineContentHelper::AvailableContentReceived(
 void AvailableOfflineContentHelper::SummaryReceived(
     base::OnceCallback<void(const std::string& content_summary_json)> callback,
     chrome::mojom::AvailableOfflineContentSummaryPtr summary) {
+  has_prefetched_content_ = false;
   if (summary->total_items == 0) {
     std::move(callback).Run("");
   } else {
+    has_prefetched_content_ = summary->has_prefetched_page;
+
     std::string json;
     base::JSONWriter::Write(AvailableContentSummaryToValue(summary), &json);
     RecordEvent(error_page::NETWORK_ERROR_PAGE_OFFLINE_CONTENT_SUMMARY_SHOWN);

chrome/renderer/net/available_offline_content_helper.h

@@ -65,6 +65,10 @@ class AvailableOfflineContentHelper {
   // only so that metrics can be recorded properly on call to LaunchItem().
   std::vector<chrome::mojom::AvailableOfflineContentPtr> fetched_content_;
 
+  // Records if the last received content message indicated that prefetched
+  // articles are available or not.
+  bool has_prefetched_content_ = false;
+
   DISALLOW_COPY_AND_ASSIGN(AvailableOfflineContentHelper);
 };
 

chrome/renderer/net/net_error_helper_core_unittest.cc

@@ -2670,7 +2670,7 @@ class FakeAvailableOfflineContentProvider
 
   MOCK_METHOD2(LaunchItem,
                void(const std::string& item_ID, const std::string& name_space));
-  MOCK_METHOD0(LaunchDownloadsPage, void());
+  MOCK_METHOD1(LaunchDownloadsPage, void(bool open_prefetched_articles_tab));
 
   void AddBinding(mojo::ScopedMessagePipeHandle handle) {
     bindings_.AddBinding(this,

components/offline_pages/core/model/offline_page_model_taskified.h

@@ -79,7 +79,6 @@ class OfflinePageModelTaskified : public OfflinePageModel,
   // OfflinePageModel implementation.
   void AddObserver(Observer* observer) override;
   void RemoveObserver(Observer* observer) override;
-
   void SavePage(const SavePageParams& save_page_params,
                 std::unique_ptr<OfflinePageArchiver> archiver,
                 content::WebContents* web_contents,
@@ -108,10 +107,8 @@ class OfflinePageModelTaskified : public OfflinePageModel,
                      MultipleOfflinePageItemCallback callback) override;
   void GetPagesByNamespace(const std::string& name_space,
                            MultipleOfflinePageItemCallback callback) override;
-  // Get all pages in the namespaces that will be removed on cache reset.
   void GetPagesRemovedOnCacheReset(
       MultipleOfflinePageItemCallback callback) override;
-  // Get all pages in the namespaces that are shown in download ui.
   void GetPagesSupportedByDownloads(
       MultipleOfflinePageItemCallback callback) override;
   void GetPagesByRequestOrigin(
@@ -130,16 +127,11 @@ class OfflinePageModelTaskified : public OfflinePageModel,
   void HasThumbnailForOfflineId(
       int64_t offline_id,
       base::OnceCallback<void(bool)> callback) override;
-
   const base::FilePath& GetInternalArchiveDirectory(
       const std::string& name_space) const override;
   bool IsArchiveInInternalDir(const base::FilePath& file_path) const override;
-
   ClientPolicyController* GetPolicyController() override;
-
   OfflineEventLogger* GetLogger() override;
-
-  // Publish an offline page from our internal directory to a public directory.
   void PublishInternalArchive(
       const OfflinePageItem& offline_page,
       std::unique_ptr<OfflinePageArchiver> archiver,