DownloadOfflineContentProvider : Some fixes

1 - Changed implementation to observe individual DownloadItems rather than
observing an AllDownloadItemNotifier::Observer, because AIDN generates duplicate
updates to the UI.
2 - Changed this class to work with in-progress download manager. This class now
depends on DownloadManagerService to get all downloads.
3 - Changed OfflineContentAggregator to a singleton for android so that it can
 work without a BrowserContext.
4 - TODO : This class is owned by DownloadCoreServiceImpl. Need to fix this
  for service manager mode.

Change-Id: I5741fb0fc9eef05415ab907b23c1fb575ef2203d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1422392
Commit-Queue: Shakti Sahu <shaktisahu@chromium.org>
Reviewed-by: Shakti Sahu <shaktisahu@chromium.org>
Reviewed-by: David Trainor <dtrainor@chromium.org>
Reviewed-by: Min Qin <qinmin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#637978}

chrome/browser/android/download/available_offline_content_provider_unittest.cc

@@ -36,11 +36,6 @@ using offline_items_collection::OfflineItemVisuals;
 using testing::_;
 const char kProviderNamespace[] = "offline_pages";
 
-std::unique_ptr<KeyedService> BuildOfflineContentAggregator(
-    content::BrowserContext* context) {
-  return std::make_unique<OfflineContentAggregator>();
-}
-
 OfflineItem UninterestingImageItem() {
   OfflineItem item;
   item.original_url = GURL("https://uninteresting");
@@ -129,11 +124,8 @@ 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*>(
-        OfflineContentAggregatorFactory::GetInstance()->SetTestingFactoryAndUse(
-            &profile_, base::BindRepeating(&BuildOfflineContentAggregator)));
+    aggregator_ =
+        OfflineContentAggregatorFactory::GetForBrowserContext(nullptr);
     aggregator_->RegisterProvider(kProviderNamespace, &content_provider_);
     content_provider_.SetVisuals({});
   }

chrome/browser/android/download/download_manager_service.cc

@@ -646,6 +646,18 @@ void DownloadManagerService::OnResumptionFailedInternal(
     resume_callback_for_testing_.Run(false);
 }
 
+void DownloadManagerService::GetAllDownloads(
+    content::DownloadManager::DownloadVector* all_items,
+    bool is_off_the_record) {
+  if (in_progress_manager_) {
+    in_progress_manager_->GetAllDownloads(all_items);
+  } else {
+    content::DownloadManager* manager = GetDownloadManager(is_off_the_record);
+    if (manager)
+      manager->GetAllDownloads(all_items);
+  }
+}
+
 download::DownloadItem* DownloadManagerService::GetDownload(
     const std::string& download_guid,
     bool is_off_the_record) {

chrome/browser/android/download/download_manager_service.h

@@ -165,6 +165,14 @@ class DownloadManagerService
   download::InProgressDownloadManager* RetriveInProgressDownloadManager(
       content::BrowserContext* context);
 
+  // Get all downloads from DownloadManager or InProgressManager.
+  void GetAllDownloads(content::DownloadManager::DownloadVector* all_items,
+                       bool is_off_the_record);
+
+  // Gets a download item from DownloadManager or InProgressManager.
+  download::DownloadItem* GetDownload(const std::string& download_guid,
+                                      bool is_off_the_record);
+
  protected:
   // Called to get the content::DownloadManager instance.
   virtual content::DownloadManager* GetDownloadManager(bool is_off_the_record);
@@ -204,10 +212,6 @@ class DownloadManagerService
 
   void OnResumptionFailedInternal(const std::string& download_guid);
 
-  // Gets a download item from DownloadManager or InProgressManager.
-  download::DownloadItem* GetDownload(const std::string& download_guid,
-                                      bool is_off_the_record);
-
   // Creates the InProgressDownloadmanager when running with ServiceManager
   // only mode.
   void CreateInProgressDownloadManager();

chrome/browser/download/download_core_service_impl.cc

@@ -12,9 +12,12 @@
 #include "chrome/browser/download/download_offline_content_provider.h"
 #include "chrome/browser/download/download_status_updater.h"
 #include "chrome/browser/download/download_ui_controller.h"
+#include "chrome/browser/download/offline_item_utils.h"
 #include "chrome/browser/history/history_service_factory.h"
+#include "chrome/browser/offline_items_collection/offline_content_aggregator_factory.h"
 #include "chrome/browser/profiles/profile.h"
 #include "components/history/core/browser/history_service.h"
+#include "components/offline_items_collection/core/offline_content_aggregator.h"
 #include "content/public/browser/download_manager.h"
 #include "extensions/buildflags/buildflags.h"
 
@@ -61,12 +64,19 @@ DownloadCoreServiceImpl::GetDownloadManagerDelegate() {
                      new DownloadHistory::HistoryAdapter(history))));
   }
 
+  download_provider_.reset(new DownloadOfflineContentProvider(
+      OfflineContentAggregatorFactory::GetForBrowserContext(
+          profile_->GetOriginalProfile()),
+      offline_items_collection::OfflineContentAggregator::CreateUniqueNameSpace(
+          OfflineItemUtils::GetDownloadNamespacePrefix(
+              profile_->IsOffTheRecord()),
+          profile_->IsOffTheRecord())));
+
   // Pass an empty delegate when constructing the DownloadUIController. The
   // default delegate does all the notifications we need.
   download_ui_.reset(new DownloadUIController(
-      manager, std::unique_ptr<DownloadUIController::Delegate>()));
-
-  download_provider_.reset(new DownloadOfflineContentProvider(manager));
+      manager, std::unique_ptr<DownloadUIController::Delegate>(),
+      download_provider_.get()));
 
 #if !defined(OS_ANDROID)
   download_shelf_controller_.reset(new DownloadShelfController(profile_));

chrome/browser/download/download_core_service_impl.h

@@ -65,6 +65,10 @@ class DownloadCoreServiceImpl : public DownloadCoreService {
 
   std::unique_ptr<DownloadHistory> download_history_;
 
+  // The download provider is the responsible for supplying offline items to the
+  // UI.
+  std::unique_ptr<DownloadOfflineContentProvider> download_provider_;
+
   // The UI controller is responsible for observing the download manager and
   // notifying the UI of any new downloads. Its lifetime matches that of the
   // associated download manager.
@@ -76,10 +80,6 @@ class DownloadCoreServiceImpl : public DownloadCoreService {
   std::unique_ptr<DownloadShelfController> download_shelf_controller_;
 #endif
 
-  // The download provider is the responsible for supplying offline items to the
-  // UI.
-  std::unique_ptr<DownloadOfflineContentProvider> download_provider_;
-
 // On Android, GET downloads are not handled by the DownloadManager.
 // Once we have extensions on android, we probably need the EventRouter
 // in ContentViewDownloadDelegate which knows about both GET and POST

chrome/browser/download/download_offline_content_provider.cc

@@ -24,6 +24,7 @@
 
 #if defined(OS_ANDROID)
 #include "chrome/browser/android/download/download_manager_bridge.h"
+#include "chrome/browser/android/download/download_manager_service.h"
 #endif
 
 using OfflineItemFilter = offline_items_collection::OfflineItemFilter;
@@ -37,7 +38,7 @@ namespace {
 // Thumbnail size used for generating thumbnails for image files.
 const int kThumbnailSizeInDP = 64;
 
-bool ShouldShowDownloadItem(const download::DownloadItem* item) {
+bool ShouldShowDownloadItem(const DownloadItem* item) {
   return !item->IsTemporary() && !item->IsTransient() && !item->IsDangerous() &&
          !item->GetTargetFilePath().empty();
 }
@@ -45,16 +46,12 @@ bool ShouldShowDownloadItem(const download::DownloadItem* item) {
 }  // namespace
 
 DownloadOfflineContentProvider::DownloadOfflineContentProvider(
-    DownloadManager* manager)
-    : manager_(manager),
-      download_notifier_(manager, this),
+    OfflineContentAggregator* aggregator,
+    const std::string& name_space)
+    : aggregator_(aggregator),
+      name_space_(name_space),
+      manager_(nullptr),
       weak_ptr_factory_(this) {
-  Profile* profile = Profile::FromBrowserContext(manager_->GetBrowserContext());
-  profile = profile->GetOriginalProfile();
-  aggregator_ = OfflineContentAggregatorFactory::GetForBrowserContext(profile);
-  bool incognito = manager_->GetBrowserContext()->IsOffTheRecord();
-  name_space_ = OfflineContentAggregator::CreateUniqueNameSpace(
-      OfflineItemUtils::GetDownloadNamespacePrefix(incognito), incognito);
   aggregator_->RegisterProvider(name_space_, this);
 }
 
@@ -62,35 +59,40 @@ DownloadOfflineContentProvider::~DownloadOfflineContentProvider() {
   aggregator_->UnregisterProvider(name_space_);
 }
 
+void DownloadOfflineContentProvider::SetDownloadManager(
+    DownloadManager* manager) {
+  manager_ = manager;
+}
+
 // TODO(shaktisahu) : Pass DownloadOpenSource.
 void DownloadOfflineContentProvider::OpenItem(LaunchLocation location,
                                               const ContentId& id) {
-  download::DownloadItem* item = manager_->GetDownloadByGuid(id.id);
+  DownloadItem* item = GetDownload(id.id);
   if (item)
     item->OpenDownload();
 }
 
 void DownloadOfflineContentProvider::RemoveItem(const ContentId& id) {
-  download::DownloadItem* item = manager_->GetDownloadByGuid(id.id);
+  DownloadItem* item = GetDownload(id.id);
   if (item)
     item->Remove();
 }
 
 void DownloadOfflineContentProvider::CancelDownload(const ContentId& id) {
-  download::DownloadItem* item = manager_->GetDownloadByGuid(id.id);
+  DownloadItem* item = GetDownload(id.id);
   if (item)
     item->Cancel(true);
 }
 
 void DownloadOfflineContentProvider::PauseDownload(const ContentId& id) {
-  download::DownloadItem* item = manager_->GetDownloadByGuid(id.id);
+  DownloadItem* item = GetDownload(id.id);
   if (item)
     item->Pause();
 }
 
 void DownloadOfflineContentProvider::ResumeDownload(const ContentId& id,
                                                     bool has_user_gesture) {
-  download::DownloadItem* item = manager_->GetDownloadByGuid(id.id);
+  DownloadItem* item = GetDownload(id.id);
   if (item)
     item->Resume(has_user_gesture);
 }
@@ -98,7 +100,7 @@ void DownloadOfflineContentProvider::ResumeDownload(const ContentId& id,
 void DownloadOfflineContentProvider::GetItemById(
     const ContentId& id,
     OfflineContentProvider::SingleItemCallback callback) {
-  DownloadItem* item = manager_->GetDownloadByGuid(id.id);
+  DownloadItem* item = GetDownload(id.id);
   auto offline_item =
       item && ShouldShowDownloadItem(item)
           ? base::make_optional(
@@ -112,7 +114,7 @@ void DownloadOfflineContentProvider::GetItemById(
 void DownloadOfflineContentProvider::GetAllItems(
     OfflineContentProvider::MultipleItemCallback callback) {
   DownloadManager::DownloadVector all_items;
-  manager_->GetAllDownloads(&all_items);
+  GetAllDownloads(&all_items);
 
   std::vector<OfflineItem> items;
   for (auto* item : all_items) {
@@ -130,7 +132,7 @@ void DownloadOfflineContentProvider::GetVisualsForItem(
     const ContentId& id,
     VisualsCallback callback) {
   // TODO(crbug.com/855330) Supply thumbnail if item is visible.
-  DownloadItem* item = manager_->GetDownloadByGuid(id.id);
+  DownloadItem* item = GetDownload(id.id);
   if (!item)
     return;
 
@@ -177,8 +179,14 @@ void DownloadOfflineContentProvider::RemoveObserver(
   observers_.RemoveObserver(observer);
 }
 
-void DownloadOfflineContentProvider::OnDownloadUpdated(DownloadManager* manager,
-                                                       DownloadItem* item) {
+void DownloadOfflineContentProvider::OnDownloadStarted(DownloadItem* item) {
+  item->RemoveObserver(this);
+  item->AddObserver(this);
+
+  OnDownloadUpdated(item);
+}
+
+void DownloadOfflineContentProvider::OnDownloadUpdated(DownloadItem* item) {
   // Wait until the target path is determined or the download is canceled.
   if (item->GetTargetFilePath().empty() &&
       item->GetState() != DownloadItem::CANCELLED)
@@ -196,8 +204,7 @@ void DownloadOfflineContentProvider::OnDownloadUpdated(DownloadManager* manager,
   UpdateObservers(item);
 }
 
-void DownloadOfflineContentProvider::OnDownloadRemoved(DownloadManager* manager,
-                                                       DownloadItem* item) {
+void DownloadOfflineContentProvider::OnDownloadRemoved(DownloadItem* item) {
   if (!ShouldShowDownloadItem(item))
     return;
 
@@ -231,6 +238,29 @@ void DownloadOfflineContentProvider::AddCompletedDownloadDone(
   UpdateObservers(item);
 }
 
+DownloadItem* DownloadOfflineContentProvider::GetDownload(
+    const std::string& download_guid) {
+#if defined(OS_ANDROID)
+  bool incognito =
+      manager_ ? manager_->GetBrowserContext()->IsOffTheRecord() : false;
+  return DownloadManagerService::GetInstance()->GetDownload(download_guid,
+                                                            incognito);
+#else
+  return manager_->GetDownloadByGuid(download_guid);
+#endif
+}
+
+void DownloadOfflineContentProvider::GetAllDownloads(
+    DownloadManager::DownloadVector* all_items) {
+#if defined(OS_ANDROID)
+  bool incognito =
+      manager_ ? manager_->GetBrowserContext()->IsOffTheRecord() : false;
+  DownloadManagerService::GetInstance()->GetAllDownloads(all_items, incognito);
+#else
+  manager_->GetAllDownloads(all_items);
+#endif
+}
+
 void DownloadOfflineContentProvider::UpdateObservers(DownloadItem* item) {
   for (auto& observer : observers_) {
     observer.OnItemUpdated(

chrome/browser/download/download_offline_content_provider.h

@@ -9,9 +9,10 @@
 #include <set>
 
 #include "base/macros.h"
-#include "components/download/content/public/all_download_item_notifier.h"
+#include "components/download/public/common/download_item.h"
 #include "components/offline_items_collection/core/offline_content_aggregator.h"
 #include "components/offline_items_collection/core/offline_content_provider.h"
+#include "content/public/browser/download_manager.h"
 
 using DownloadItem = download::DownloadItem;
 using DownloadManager = content::DownloadManager;
@@ -24,15 +25,19 @@ using LaunchLocation = offline_items_collection::LaunchLocation;
 
 class SkBitmap;
 
-// This class handles the task of observing a single DownloadManager and
-// notifies UI about updates about various downloads.
-class DownloadOfflineContentProvider
-    : public OfflineContentProvider,
-      public download::AllDownloadItemNotifier::Observer {
+// This class handles the task of observing the downloads associated with a
+// single DownloadManager (or in-progress download manager in service manager
+// only mode) and notifies UI about updates about various downloads.
+class DownloadOfflineContentProvider : public OfflineContentProvider,
+                                       public download::DownloadItem::Observer {
  public:
-  explicit DownloadOfflineContentProvider(DownloadManager* manager);
+  explicit DownloadOfflineContentProvider(OfflineContentAggregator* aggregator,
+                                          const std::string& name_space);
   ~DownloadOfflineContentProvider() override;
 
+  // Should be called when a DownloadManager is available.
+  void SetDownloadManager(DownloadManager* manager);
+
   // OfflineContentProvider implmentation.
   void OpenItem(LaunchLocation location, const ContentId& id) override;
   void RemoveItem(const ContentId& id) override;
@@ -52,11 +57,17 @@ class DownloadOfflineContentProvider
   void AddObserver(OfflineContentProvider::Observer* observer) override;
   void RemoveObserver(OfflineContentProvider::Observer* observer) override;
 
+  // Entry point for associating this class with a download item. Must be called
+  // for all new and in-progress downloads, after which this class will start
+  // observing the given download.
+  void OnDownloadStarted(DownloadItem* download_item);
+
  private:
-  // AllDownloadItemNotifier::Observer methods.
-  void OnDownloadUpdated(DownloadManager* manager, DownloadItem* item) override;
-  void OnDownloadRemoved(DownloadManager* manager, DownloadItem* item) override;
+  void OnDownloadUpdated(DownloadItem* item) override;
+  void OnDownloadRemoved(DownloadItem* item) override;
 
+  void GetAllDownloads(DownloadManager::DownloadVector* all_items);
+  DownloadItem* GetDownload(const std::string& download_guid);
   void OnThumbnailRetrieved(const ContentId& id,
                             VisualsCallback callback,
                             const SkBitmap& bitmap);
@@ -64,11 +75,10 @@ class DownloadOfflineContentProvider
   void AddCompletedDownloadDone(DownloadItem* item, int64_t system_download_id);
   void UpdateObservers(DownloadItem* item);
 
-  DownloadManager* manager_;
-  download::AllDownloadItemNotifier download_notifier_;
   base::ObserverList<OfflineContentProvider::Observer>::Unchecked observers_;
   OfflineContentAggregator* aggregator_;
   std::string name_space_;
+  DownloadManager* manager_;
 
   base::WeakPtrFactory<DownloadOfflineContentProvider> weak_ptr_factory_;
 

chrome/browser/download/download_ui_controller.cc

@@ -112,9 +112,13 @@ void DownloadShelfUIControllerDelegate::OnNewDownloadReady(
 DownloadUIController::Delegate::~Delegate() {
 }
 
-DownloadUIController::DownloadUIController(content::DownloadManager* manager,
-                                           std::unique_ptr<Delegate> delegate)
-    : download_notifier_(manager, this), delegate_(std::move(delegate)) {
+DownloadUIController::DownloadUIController(
+    content::DownloadManager* manager,
+    std::unique_ptr<Delegate> delegate,
+    DownloadOfflineContentProvider* provider)
+    : download_notifier_(manager, this),
+      delegate_(std::move(delegate)),
+      download_provider_(provider) {
 #if defined(OS_ANDROID)
   if (!delegate_)
     delegate_.reset(new AndroidUIControllerDelegate());
@@ -202,4 +206,6 @@ void DownloadUIController::OnDownloadUpdated(content::DownloadManager* manager,
 
   DownloadItemModel(item).SetWasUINotified(true);
   delegate_->OnNewDownloadReady(item);
+  if (download_provider_)
+    download_provider_->OnDownloadStarted(item);
 }

chrome/browser/download/download_ui_controller.h

@@ -9,6 +9,7 @@
 #include <set>
 
 #include "base/macros.h"
+#include "chrome/browser/download/download_offline_content_provider.h"
 #include "components/download/content/public/all_download_item_notifier.h"
 
 // This class handles the task of observing a single DownloadManager for
@@ -36,7 +37,8 @@ class DownloadUIController
   //
   // Currently explicit delegates are only used for testing.
   DownloadUIController(content::DownloadManager* manager,
-                       std::unique_ptr<Delegate> delegate);
+                       std::unique_ptr<Delegate> delegate,
+                       DownloadOfflineContentProvider* provider);
 
   ~DownloadUIController() override;
 
@@ -49,6 +51,7 @@ class DownloadUIController
   download::AllDownloadItemNotifier download_notifier_;
 
   std::unique_ptr<Delegate> delegate_;
+  DownloadOfflineContentProvider* download_provider_;
 
   DISALLOW_COPY_AND_ASSIGN(DownloadUIController);
 };

chrome/browser/download/download_ui_controller_unittest.cc

@@ -97,6 +97,8 @@ class DownloadUIControllerTest : public ChromeRenderViewHostTestHarness {
   // delegate results in the DownloadItem* being stored in |notified_item_|.
   std::unique_ptr<DownloadUIController::Delegate> GetTestDelegate();
 
+  DownloadOfflineContentProvider* GetDownloadProvider() { return nullptr; }
+
   MockDownloadManager* manager() { return manager_.get(); }
 
   // Returns the DownloadManager::Observer registered by a test case. This is
@@ -269,7 +271,8 @@ DownloadUIControllerTest::GetTestDelegate() {
 // a non-empty path.  I.e. once the download target has been determined.
 TEST_F(DownloadUIControllerTest, DownloadUIController_NotifyBasic) {
   std::unique_ptr<MockDownloadItem> item(CreateMockInProgressDownload());
-  DownloadUIController controller(manager(), GetTestDelegate());
+  DownloadUIController controller(manager(), GetTestDelegate(),
+                                  GetDownloadProvider());
   EXPECT_CALL(*item, GetTargetFilePath())
       .WillOnce(ReturnRefOfCopy(base::FilePath()));
 
@@ -291,7 +294,8 @@ TEST_F(DownloadUIControllerTest, DownloadUIController_NotifyBasic) {
 // A download that's created in an interrupted state should also be displayed.
 TEST_F(DownloadUIControllerTest, DownloadUIController_NotifyBasic_Interrupted) {
   std::unique_ptr<MockDownloadItem> item = CreateMockInProgressDownload();
-  DownloadUIController controller(manager(), GetTestDelegate());
+  DownloadUIController controller(manager(), GetTestDelegate(),
+                                  GetDownloadProvider());
   EXPECT_CALL(*item, GetState())
       .WillRepeatedly(Return(download::DownloadItem::INTERRUPTED));
 
@@ -305,7 +309,8 @@ TEST_F(DownloadUIControllerTest, DownloadUIController_NotifyBasic_Interrupted) {
 // additional OnDownloadUpdated() notification.
 TEST_F(DownloadUIControllerTest, DownloadUIController_NotifyReadyOnCreate) {
   std::unique_ptr<MockDownloadItem> item(CreateMockInProgressDownload());
-  DownloadUIController controller(manager(), GetTestDelegate());
+  DownloadUIController controller(manager(), GetTestDelegate(),
+                                  GetDownloadProvider());
 
   ASSERT_TRUE(manager_observer());
   manager_observer()->OnDownloadCreated(manager(), item.get());
@@ -314,7 +319,8 @@ TEST_F(DownloadUIControllerTest, DownloadUIController_NotifyReadyOnCreate) {
 
 // The UI shouldn't be notified of downloads that were restored from history.
 TEST_F(DownloadUIControllerTest, DownloadUIController_HistoryDownload) {
-  DownloadUIController controller(manager(), GetTestDelegate());
+  DownloadUIController controller(manager(), GetTestDelegate(),
+                                  GetDownloadProvider());
   // DownloadHistory should already have been created. It performs a query of
   // existing downloads upon creation. We'll use the callback to inject a
   // history download.

chrome/browser/offline_items_collection/offline_content_aggregator_factory.cc

@@ -5,11 +5,17 @@
 #include "chrome/browser/offline_items_collection/offline_content_aggregator_factory.h"
 
 #include "base/memory/singleton.h"
+#include "build/build_config.h"
 #include "chrome/browser/profiles/incognito_helpers.h"
 #include "components/keyed_service/content/browser_context_dependency_manager.h"
 #include "components/offline_items_collection/core/offline_content_aggregator.h"
 #include "content/public/browser/browser_context.h"
 
+#if defined(OS_ANDROID)
+static offline_items_collection::OfflineContentAggregator*
+    g_offline_content_aggregator = nullptr;
+#endif
+
 // static
 OfflineContentAggregatorFactory*
 OfflineContentAggregatorFactory::GetInstance() {
@@ -20,8 +26,16 @@ OfflineContentAggregatorFactory::GetInstance() {
 offline_items_collection::OfflineContentAggregator*
 OfflineContentAggregatorFactory::GetForBrowserContext(
     content::BrowserContext* context) {
+#if defined(OS_ANDROID)
+  if (!g_offline_content_aggregator) {
+    g_offline_content_aggregator =
+        new offline_items_collection::OfflineContentAggregator();
+  }
+  return g_offline_content_aggregator;
+#else
   return static_cast<offline_items_collection::OfflineContentAggregator*>(
       GetInstance()->GetServiceForBrowserContext(context, true));
+#endif
 }
 
 OfflineContentAggregatorFactory::OfflineContentAggregatorFactory()