More OfflineItemModel implementation

This CL makes OfflineItemModel inherit from DownloadUIModel

Bug: 881499
Change-Id: Ic7591f5ce7f5f5243cfeb8ff89ad1e77aa1b78db
Reviewed-on: https://chromium-review.googlesource.com/1237134
Commit-Queue: Min Qin <qinmin@chromium.org>
Reviewed-by: Shakti Sahu <shaktisahu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#593030}

chrome/browser/download/download_item_model.cc

@@ -303,9 +303,14 @@ base::string16 InterruptReasonMessage(
 // DownloadItemModel
 
 DownloadItemModel::DownloadItemModel(DownloadItem* download)
-    : download_(download) {}
+    : download_(download) {
+  download_->AddObserver(this);
+}
 
-DownloadItemModel::~DownloadItemModel() {}
+DownloadItemModel::~DownloadItemModel() {
+  if (download_)
+    download_->RemoveObserver(this);
+}
 
 ContentId DownloadItemModel::GetContentId() const {
   bool off_the_record = content::DownloadItemUtils::GetBrowserContext(download_)
@@ -314,16 +319,6 @@ ContentId DownloadItemModel::GetContentId() const {
                    download_->GetGuid());
 }
 
-void DownloadItemModel::AddObserver(DownloadUIModel::Observer* observer) {
-  DownloadUIModel::AddObserver(observer);
-  download_->AddObserver(this);
-}
-
-void DownloadItemModel::RemoveObserver(DownloadUIModel::Observer* observer) {
-  DownloadUIModel::RemoveObserver(observer);
-  download_->RemoveObserver(this);
-}
-
 base::string16 DownloadItemModel::GetInterruptReasonText() const {
   if (download_->GetState() != DownloadItem::INTERRUPTED ||
       download_->GetLastReason() ==
@@ -832,6 +827,7 @@ void DownloadItemModel::OnDownloadOpened(DownloadItem* download) {
 void DownloadItemModel::OnDownloadDestroyed(DownloadItem* download) {
   for (auto& obs : observers_)
     obs.OnDownloadDestroyed();
+  download_ = nullptr;
 }
 
 base::string16 DownloadItemModel::GetInProgressStatusString() const {

chrome/browser/download/download_item_model.h

@@ -31,8 +31,6 @@ class DownloadItemModel : public DownloadUIModel,
   ~DownloadItemModel() override;
 
   // DownloadUIModel implementation.
-  void AddObserver(DownloadUIModel::Observer* observer) override;
-  void RemoveObserver(DownloadUIModel::Observer* observer) override;
   ContentId GetContentId() const override;
   base::string16 GetInterruptReasonText() const override;
   base::string16 GetStatusText() const override;

chrome/browser/download/download_ui_model.h

@@ -10,6 +10,7 @@
 #include "base/files/file_path.h"
 #include "base/macros.h"
 #include "base/observer_list.h"
+#include "base/sequenced_task_runner.h"
 #include "base/strings/string16.h"
 #include "build/build_config.h"
 #include "chrome/browser/download/download_commands.h"
@@ -27,6 +28,9 @@ using offline_items_collection::ContentId;
 // with a download.
 class DownloadUIModel {
  public:
+  using DownloadUIModelPtr =
+      std::unique_ptr<DownloadUIModel, base::OnTaskRunnerDeleter>;
+
   DownloadUIModel();
   virtual ~DownloadUIModel();
 
@@ -40,8 +44,8 @@ class DownloadUIModel {
     virtual ~Observer() {}
   };
 
-  virtual void AddObserver(Observer* observer);
-  virtual void RemoveObserver(Observer* observer);
+  void AddObserver(Observer* observer);
+  void RemoveObserver(Observer* observer);
 
   // Returns the content id associated with this download.
   virtual ContentId GetContentId() const;

chrome/browser/download/notification/download_item_notification.cc

@@ -234,6 +234,8 @@ void DownloadItemNotification::OnDownloadDestroyed() {
   Close(false);
 
   observer_->OnDownloadDestroyed(item_->GetContentId());
+
+  item_.reset();
 }
 
 void DownloadItemNotification::DisablePopup() {
@@ -331,6 +333,11 @@ void DownloadItemNotification::Click(
   }
 }
 
+void DownloadItemNotification::Shutdown() {
+    if (item_)
+      item_->RemoveObserver(this);
+}
+
 std::string DownloadItemNotification::GetNotificationId() const {
   return item_->GetContentId().id;
 }

chrome/browser/download/notification/download_item_notification.h

@@ -8,6 +8,7 @@
 #include <memory>
 
 #include "base/macros.h"
+#include "base/sequenced_task_runner.h"
 #include "base/strings/string_number_conversions.h"
 #include "chrome/browser/download/download_commands.h"
 #include "chrome/browser/download/download_ui_model.h"
@@ -33,8 +34,11 @@ class DownloadItemNotification : public ImageDecoder::ImageRequest,
                                  public message_center::NotificationObserver,
                                  public DownloadUIModel::Observer {
  public:
-  explicit DownloadItemNotification(Profile* profile,
-                                    std::unique_ptr<DownloadUIModel> item);
+  using DownloadItemNotificationPtr =
+      std::unique_ptr<DownloadItemNotification, base::OnTaskRunnerDeleter>;
+
+  DownloadItemNotification(Profile* profile,
+                           std::unique_ptr<DownloadUIModel> item);
   ~DownloadItemNotification() override;
 
   // Observer for this notification.
@@ -60,6 +64,7 @@ class DownloadItemNotification : public ImageDecoder::ImageRequest,
   void Click(const base::Optional<int>& button_index,
              const base::Optional<base::string16>& reply) override;
 
+  void Shutdown();
  private:
   friend class test::DownloadItemNotificationTest;
 

chrome/browser/download/notification/download_item_notification_unittest.cc

@@ -123,8 +123,9 @@ class DownloadItemNotificationTest : public testing::Test {
         OfflineItemUtils::GetDownloadNamespace(profile_->IsOffTheRecord()),
         download_item_->GetGuid());
     download_notification_manager_->OnNewDownloadReady(download_item_.get());
+
     download_item_notification_ =
-        download_notification_manager_->items_[id].get();
+        download_notification_manager_->items_.at(id).get();
     NotificationDisplayServiceFactory::GetForProfile(profile_)->Display(
         NotificationHandler::Type::TRANSIENT,
         *download_item_notification_->notification_);

chrome/browser/download/notification/download_notification_manager.cc

@@ -5,12 +5,19 @@
 #include "chrome/browser/download/notification/download_notification_manager.h"
 
 #include "chrome/browser/download/download_item_model.h"
-#include "chrome/browser/download/notification/download_item_notification.h"
 
 DownloadNotificationManager::DownloadNotificationManager(Profile* profile)
     : profile_(profile) {}
 
-DownloadNotificationManager::~DownloadNotificationManager() = default;
+DownloadNotificationManager::~DownloadNotificationManager() {
+  for (auto& item : items_) {
+    DownloadItemNotification* download_notification = item.second.get();
+    DownloadUIModel* model = download_notification->GetDownload();
+    if (model->GetState() == download::DownloadItem::IN_PROGRESS)
+      download_notification->DisablePopup();
+    download_notification->Shutdown();
+  }
+}
 
 void DownloadNotificationManager::OnNewDownloadReady(
     download::DownloadItem* item) {
@@ -25,9 +32,11 @@ void DownloadNotificationManager::OnNewDownloadReady(
   std::unique_ptr<DownloadUIModel> model =
       std::make_unique<DownloadItemModel>(item);
   ContentId contentId = model->GetContentId();
-  items_[contentId] =
-      std::make_unique<DownloadItemNotification>(profile_, std::move(model));
-  items_[contentId]->SetObserver(this);
+  DownloadItemNotification::DownloadItemNotificationPtr notification(
+      new DownloadItemNotification(profile_, std::move(model)),
+      base::OnTaskRunnerDeleter(base::ThreadTaskRunnerHandle::Get()));
+  notification->SetObserver(this);
+  items_.emplace(contentId, std::move(notification));
 }
 
 void DownloadNotificationManager::OnDownloadDestroyed(

chrome/browser/download/notification/download_notification_manager.h

@@ -33,7 +33,8 @@ class DownloadNotificationManager : public DownloadUIController::Delegate,
   friend class test::DownloadItemNotificationTest;
 
   Profile* profile_;
-  std::map<ContentId, std::unique_ptr<DownloadItemNotification>> items_;
+  std::map<ContentId, DownloadItemNotification::DownloadItemNotificationPtr>
+      items_;
 
   DISALLOW_COPY_AND_ASSIGN(DownloadNotificationManager);
 };

chrome/browser/download/offline_item_model.cc

@@ -5,21 +5,38 @@
 #include "chrome/browser/download/offline_item_model.h"
 
 #include "chrome/browser/download/offline_item_model_manager.h"
+#include "chrome/browser/offline_items_collection/offline_content_aggregator_factory.h"
+#include "components/offline_items_collection/core/offline_content_aggregator.h"
 
-OfflineItemModel::OfflineItemModel(
-    OfflineItemModelManager* manager,
-    const offline_items_collection::OfflineItem& offline_item)
-    : manager_(manager), offline_item_(offline_item) {}
+using offline_items_collection::ContentId;
+using offline_items_collection::OfflineItem;
 
-OfflineItemModel::~OfflineItemModel() = default;
+OfflineItemModel::OfflineItemModel(OfflineItemModelManager* manager,
+                                   const OfflineItem& offline_item)
+    : manager_(manager),
+      offline_item_(std::make_unique<OfflineItem>(offline_item)) {
+  offline_items_collection::OfflineContentAggregator* aggregator =
+      OfflineContentAggregatorFactory::GetForBrowserContext(
+          manager_->browser_context());
+  offline_item_observer_ =
+      std::make_unique<FilteredOfflineItemObserver>(aggregator);
+  offline_item_observer_->AddObserver(offline_item_->id, this);
+}
+
+OfflineItemModel::~OfflineItemModel() {
+  if (offline_item_)
+    offline_item_observer_->RemoveObserver(offline_item_->id, this);
+}
 
 int64_t OfflineItemModel::GetCompletedBytes() const {
-  return offline_item_.received_bytes;
+  return offline_item_ ? offline_item_->received_bytes : 0;
 }
 
 int64_t OfflineItemModel::GetTotalBytes() const {
-  return offline_item_.total_size_bytes > 0 ? offline_item_.total_size_bytes
-                                            : 0;
+  if (!offline_item_)
+    return 0;
+  return offline_item_->total_size_bytes > 0 ? offline_item_->total_size_bytes
+                                             : 0;
 }
 
 int OfflineItemModel::PercentComplete() const {
@@ -31,12 +48,24 @@ int OfflineItemModel::PercentComplete() const {
 
 bool OfflineItemModel::WasUINotified() const {
   const OfflineItemModelData* data =
-      manager_->GetOrCreateOfflineItemModelData(offline_item_.id);
+      manager_->GetOrCreateOfflineItemModelData(offline_item_->id);
   return data->was_ui_notified_;
 }
 
 void OfflineItemModel::SetWasUINotified(bool was_ui_notified) {
   OfflineItemModelData* data =
-      manager_->GetOrCreateOfflineItemModelData(offline_item_.id);
+      manager_->GetOrCreateOfflineItemModelData(offline_item_->id);
   data->was_ui_notified_ = was_ui_notified;
 }
+
+void OfflineItemModel::OnItemRemoved(const ContentId& id) {
+  for (auto& obs : observers_)
+    obs.OnDownloadDestroyed();
+  offline_item_.reset();
+}
+
+void OfflineItemModel::OnItemUpdated(const OfflineItem& item) {
+  offline_item_ = std::make_unique<OfflineItem>(item);
+  for (auto& obs : observers_)
+    obs.OnDownloadUpdated();
+}

chrome/browser/download/offline_item_model.h

@@ -5,13 +5,19 @@
 #ifndef CHROME_BROWSER_DOWNLOAD_OFFLINE_ITEM_MODEL_H_
 #define CHROME_BROWSER_DOWNLOAD_OFFLINE_ITEM_MODEL_H_
 
+#include <memory>
+
 #include "chrome/browser/download/download_ui_model.h"
+#include "components/offline_items_collection/core/filtered_offline_item_observer.h"
 #include "components/offline_items_collection/core/offline_item.h"
 
 class OfflineItemModelManager;
 
+using offline_items_collection::FilteredOfflineItemObserver;
+
 // Implementation of DownloadUIModel that wrappers around a |OfflineItem|.
-class OfflineItemModel : public DownloadUIModel {
+class OfflineItemModel : public DownloadUIModel,
+                         public FilteredOfflineItemObserver::Observer {
  public:
   // Constructs a OfflineItemModel.
   OfflineItemModel(OfflineItemModelManager* manager,
@@ -26,9 +32,15 @@ class OfflineItemModel : public DownloadUIModel {
   void SetWasUINotified(bool should_notify) override;
 
  private:
+  // FilteredOfflineItemObserver::Observer overrides.
+  void OnItemRemoved(const offline_items_collection::ContentId& id) override;
+  void OnItemUpdated(
+      const offline_items_collection::OfflineItem& item) override;
+
   OfflineItemModelManager* manager_;
 
-  offline_items_collection::OfflineItem offline_item_;
+  std::unique_ptr<FilteredOfflineItemObserver> offline_item_observer_;
+  std::unique_ptr<offline_items_collection::OfflineItem> offline_item_;
 
   DISALLOW_COPY_AND_ASSIGN(OfflineItemModel);
 };

chrome/browser/download/offline_item_model_manager.cc

@@ -4,7 +4,9 @@
 
 #include "chrome/browser/download/offline_item_model_manager.h"
 
-OfflineItemModelManager::OfflineItemModelManager() = default;
+OfflineItemModelManager::OfflineItemModelManager(
+    content::BrowserContext* browser_context)
+    : browser_context_(browser_context) {}
 
 OfflineItemModelManager::~OfflineItemModelManager() = default;
 

chrome/browser/download/offline_item_model_manager.h

@@ -11,13 +11,17 @@
 #include "components/keyed_service/core/keyed_service.h"
 #include "components/offline_items_collection/core/offline_item.h"
 
+namespace content {
+class BrowserContext;
+}  // namespace content
+
 using ContentId = offline_items_collection::ContentId;
 
 // Class for managing all the OfflineModels for a profile.
 class OfflineItemModelManager : public KeyedService {
  public:
   // Constructs a OfflineItemModelManager.
-  OfflineItemModelManager();
+  explicit OfflineItemModelManager(content::BrowserContext* browser_context);
   ~OfflineItemModelManager() override;
 
   // Returns the OfflineItemModel for the ContentId, if not found, an empty
@@ -26,7 +30,10 @@ class OfflineItemModelManager : public KeyedService {
 
   void RemoveOfflineItemModelData(const ContentId& id);
 
+  content::BrowserContext* browser_context() { return browser_context_; }
+
  private:
+  content::BrowserContext* browser_context_;
   std::map<ContentId, std::unique_ptr<OfflineItemModelData>>
       offline_item_model_data_;
 

chrome/browser/download/offline_item_model_manager_factory.cc

@@ -30,5 +30,5 @@ OfflineItemModelManagerFactory::~OfflineItemModelManagerFactory() = default;
 
 KeyedService* OfflineItemModelManagerFactory::BuildServiceInstanceFor(
     content::BrowserContext* context) const {
-  return new OfflineItemModelManager();
+  return new OfflineItemModelManager(context);
 }

chrome/browser/ui/views/download/download_item_view.cc

@@ -127,7 +127,7 @@ class SeparatorBorder : public views::FocusableBorder {
 
 }  // namespace
 
-DownloadItemView::DownloadItemView(std::unique_ptr<DownloadUIModel> download,
+DownloadItemView::DownloadItemView(DownloadUIModel::DownloadUIModelPtr download,
                                    DownloadShelfView* parent,
                                    views::View* accessible_alert)
     : shelf_(parent),

chrome/browser/ui/views/download/download_item_view.h

@@ -64,7 +64,7 @@ class DownloadItemView : public views::InkDropHostView,
                          public DownloadUIModel::Observer,
                          public gfx::AnimationDelegate {
  public:
-  DownloadItemView(std::unique_ptr<DownloadUIModel> download,
+  DownloadItemView(DownloadUIModel::DownloadUIModelPtr download,
                    DownloadShelfView* parent,
                    views::View* accessible_alert);
   ~DownloadItemView() override;
@@ -351,7 +351,7 @@ class DownloadItemView : public views::InkDropHostView,
   base::CancelableTaskTracker cancelable_task_tracker_;
 
   // A model class to control the status text we display.
-  std::unique_ptr<DownloadUIModel> model_;
+  DownloadUIModel::DownloadUIModelPtr model_;
 
   // Animation for download complete.
   std::unique_ptr<gfx::SlideAnimation> complete_animation_;

chrome/browser/ui/views/download/download_shelf_view.cc

@@ -120,8 +120,11 @@ void DownloadShelfView::AddDownloadView(DownloadItemView* view) {
 }
 
 void DownloadShelfView::DoAddDownload(DownloadItem* download) {
-  AddDownloadView(new DownloadItemView(
-      std::make_unique<DownloadItemModel>(download), this, accessible_alert_));
+  DownloadUIModel::DownloadUIModelPtr model(
+      new DownloadItemModel(download),
+      base::OnTaskRunnerDeleter(base::ThreadTaskRunnerHandle::Get()));
+  AddDownloadView(
+      new DownloadItemView(std::move(model), this, accessible_alert_));
 }
 
 void DownloadShelfView::MouseMovedOutOfHost() {