Send updates for FAILED downloads only once

When download offline content provider is enabled, we send redundant
FAILED updates. However the notification layer removes it from the shared
preferences the first time it sees a FAILED download. Hence it will
result in two different notifications. The solution is to treat COMPLETED
and FAILED as terminal downloads and filter out duplicate updates for them.

Bug: 1003550
Change-Id: Ief7c4450d644259352d3626e4f9dbd7b15362091
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1801228
Commit-Queue: Shakti Sahu <shaktisahu@chromium.org>
Reviewed-by: Min Qin <qinmin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#696989}

chrome/browser/download/download_core_service_impl.cc

@@ -80,8 +80,7 @@ DownloadCoreServiceImpl::GetDownloadManagerDelegate() {
   // 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));
+      manager, std::unique_ptr<DownloadUIController::Delegate>()));
 
 #if !defined(OS_ANDROID)
   download_shelf_controller_.reset(new DownloadShelfController(profile_));

chrome/browser/download/download_offline_content_provider.cc

@@ -456,23 +456,19 @@ void DownloadOfflineContentProvider::OnDownloadUpdated(DownloadItem* item) {
     return;
 
   UpdateDelta update_delta;
-  if (item->GetState() == DownloadItem::COMPLETE) {
-    update_delta.state_changed = completed_downloads_.find(item->GetGuid()) ==
-                                 completed_downloads_.end();
-
+  auto offline_item = OfflineItemUtils::CreateOfflineItem(name_space_, item);
+  if (offline_item.state == OfflineItemState::COMPLETE ||
+      offline_item.state == OfflineItemState::FAILED ||
+      offline_item.state == OfflineItemState::CANCELLED) {
     // TODO(crbug.com/938152): May be move this to DownloadItem.
     // Never call this for completed downloads from history.
-    if (completed_downloads_.find(item->GetGuid()) !=
-        completed_downloads_.end()) {
-      return;
-    }
-
-    completed_downloads_.insert(item->GetGuid());
+    item->RemoveObserver(this);
 
-    AddCompletedDownload(item);
+    update_delta.state_changed = true;
+    if (item->GetState() == DownloadItem::COMPLETE)
+      AddCompletedDownload(item);
   }
 
-  auto offline_item = OfflineItemUtils::CreateOfflineItem(name_space_, item);
   UpdateObservers(offline_item, update_delta);
 }
 
@@ -489,10 +485,6 @@ void DownloadOfflineContentProvider::OnDownloadRemoved(DownloadItem* item) {
     observer.OnItemRemoved(contentId);
 }
 
-void DownloadOfflineContentProvider::OnDownloadDestroyed(DownloadItem* item) {
-  completed_downloads_.erase(item->GetGuid());
-}
-
 void DownloadOfflineContentProvider::AddCompletedDownload(DownloadItem* item) {
 #if defined(OS_ANDROID)
   DownloadManagerBridge::AddCompletedDownload(

chrome/browser/download/download_offline_content_provider.h

@@ -90,7 +90,6 @@ class DownloadOfflineContentProvider
   // DownloadItem::Observer overrides
   void OnDownloadUpdated(DownloadItem* item) override;
   void OnDownloadRemoved(DownloadItem* item) override;
-  void OnDownloadDestroyed(DownloadItem* download) override;
 
  private:
   enum class State {
@@ -129,8 +128,6 @@ class DownloadOfflineContentProvider
   std::string name_space_;
   SimpleDownloadManagerCoordinator* manager_;
 
-  // Tracks the completed downloads in the current session.
-  std::set<std::string> completed_downloads_;
   std::unique_ptr<download::AllDownloadEventNotifier::Observer>
       all_download_observer_;
   bool checked_for_externally_removed_downloads_;

chrome/browser/download/download_ui_controller.cc

@@ -111,13 +111,9 @@ void DownloadShelfUIControllerDelegate::OnNewDownloadReady(
 DownloadUIController::Delegate::~Delegate() {
 }
 
-DownloadUIController::DownloadUIController(
-    content::DownloadManager* manager,
-    std::unique_ptr<Delegate> delegate,
-    DownloadOfflineContentProvider* provider)
-    : download_notifier_(manager, this),
-      delegate_(std::move(delegate)),
-      download_provider_(provider) {
+DownloadUIController::DownloadUIController(content::DownloadManager* manager,
+                                           std::unique_ptr<Delegate> delegate)
+    : download_notifier_(manager, this), delegate_(std::move(delegate)) {
 #if defined(OS_ANDROID)
   if (!delegate_)
     delegate_.reset(new AndroidUIControllerDelegate());
@@ -207,6 +203,4 @@ 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,7 +9,6 @@
 #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
@@ -37,8 +36,7 @@ class DownloadUIController
   //
   // Currently explicit delegates are only used for testing.
   DownloadUIController(content::DownloadManager* manager,
-                       std::unique_ptr<Delegate> delegate,
-                       DownloadOfflineContentProvider* provider);
+                       std::unique_ptr<Delegate> delegate);
 
   ~DownloadUIController() override;
 
@@ -51,7 +49,6 @@ 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,8 +97,6 @@ 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
@@ -274,8 +272,7 @@ 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(),
-                                  GetDownloadProvider());
+  DownloadUIController controller(manager(), GetTestDelegate());
   EXPECT_CALL(*item, GetTargetFilePath())
       .WillOnce(ReturnRefOfCopy(base::FilePath()));
 
@@ -297,8 +294,7 @@ 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(),
-                                  GetDownloadProvider());
+  DownloadUIController controller(manager(), GetTestDelegate());
   EXPECT_CALL(*item, GetState())
       .WillRepeatedly(Return(download::DownloadItem::INTERRUPTED));
 
@@ -312,8 +308,7 @@ TEST_F(DownloadUIControllerTest, DownloadUIController_NotifyBasic_Interrupted) {
 // additional OnDownloadUpdated() notification.
 TEST_F(DownloadUIControllerTest, DownloadUIController_NotifyReadyOnCreate) {
   std::unique_ptr<MockDownloadItem> item(CreateMockInProgressDownload());
-  DownloadUIController controller(manager(), GetTestDelegate(),
-                                  GetDownloadProvider());
+  DownloadUIController controller(manager(), GetTestDelegate());
 
   ASSERT_TRUE(manager_observer());
   manager_observer()->OnDownloadCreated(manager(), item.get());
@@ -322,8 +317,7 @@ 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(),
-                                  GetDownloadProvider());
+  DownloadUIController controller(manager(), GetTestDelegate());
   // 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.