Revert "Download Shelf : Connect OfflineItem to the shelf"

This reverts commit e6f5eba3.

Reason for revert: looks related to the Win10 Test flakiness reported by findit.

Original change's description:
> Download Shelf : Connect OfflineItem to the shelf
> 
> This CL connects offline item with the download shelf. Some of
> the OfflineItemModel methods are still not implemented.
> 
> Bug: 881499
> Change-Id: I2f0cd71815a08bcddfca753378543749389d78db
> Reviewed-on: https://chromium-review.googlesource.com/1239875
> Commit-Queue: Shakti Sahu <shaktisahu@chromium.org>
> Reviewed-by: Carlos Knippschild <carlosk@chromium.org>
> Reviewed-by: Min Qin <qinmin@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#594471}

TBR=qinmin@chromium.org,carlosk@chromium.org,shaktisahu@chromium.org

Change-Id: I98129d5eaca139d27ab7bb15f3ca194efb1331c6
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 881499, 889790
Reviewed-on: https://chromium-review.googlesource.com/1249068Reviewed-by: Marina Ciocea <marinaciocea@chromium.org>
Commit-Queue: Marina Ciocea <marinaciocea@chromium.org>
Cr-Commit-Position: refs/heads/master@{#594697}

chrome/browser/download/download_core_service_impl.cc

@@ -68,10 +68,6 @@ DownloadCoreServiceImpl::GetDownloadManagerDelegate() {
 
   download_provider_.reset(new DownloadOfflineContentProvider(manager));
 
-#if !defined(OS_ANDROID)
-  download_shelf_controller_.reset(new DownloadShelfController(profile_));
-#endif
-
   // Include this download manager in the set monitored by the
   // global status updater.
   DCHECK(g_browser_process->download_status_updater());

chrome/browser/download/download_core_service_impl.h

@@ -13,10 +13,6 @@
 #include "components/keyed_service/core/keyed_service.h"
 #include "extensions/buildflags/buildflags.h"
 
-#if !defined(OS_ANDROID)
-#include "chrome/browser/download/download_shelf_controller.h"
-#endif
-
 class ChromeDownloadManagerDelegate;
 class DownloadHistory;
 class DownloadOfflineContentProvider;
@@ -72,10 +68,6 @@ class DownloadCoreServiceImpl : public DownloadCoreService {
   // should be destroyed before the latter.
   std::unique_ptr<DownloadUIController> download_ui_;
 
-#if !defined(OS_ANDROID)
-  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_;

chrome/browser/download/download_item_model.cc

@@ -114,15 +114,6 @@ DownloadItemModelData::DownloadItemModelData()
 // -----------------------------------------------------------------------------
 // DownloadItemModel
 
-// static
-DownloadUIModel::DownloadUIModelPtr DownloadItemModel::Wrap(
-    download::DownloadItem* download) {
-  DownloadUIModel::DownloadUIModelPtr model(
-      new DownloadItemModel(download),
-      base::OnTaskRunnerDeleter(base::ThreadTaskRunnerHandle::Get()));
-  return model;
-}
-
 DownloadItemModel::DownloadItemModel(DownloadItem* download)
     : download_(download) {
   download_->AddObserver(this);

chrome/browser/download/download_item_model.h

@@ -21,8 +21,6 @@
 class DownloadItemModel : public DownloadUIModel,
                           public download::DownloadItem::Observer {
  public:
-  static DownloadUIModelPtr Wrap(download::DownloadItem* download);
-
   // Constructs a DownloadItemModel. The caller must ensure that |download|
   // outlives this object.
   explicit DownloadItemModel(download::DownloadItem* download);

chrome/browser/download/download_shelf.cc

@@ -17,19 +17,12 @@
 #include "chrome/browser/download/download_core_service_factory.h"
 #include "chrome/browser/download/download_item_model.h"
 #include "chrome/browser/download/download_started_animation.h"
-#include "chrome/browser/download/offline_item_model.h"
-#include "chrome/browser/download/offline_item_model_manager.h"
-#include "chrome/browser/download/offline_item_model_manager_factory.h"
-#include "chrome/browser/download/offline_item_utils.h"
-#include "chrome/browser/offline_items_collection/offline_content_aggregator_factory.h"
 #include "chrome/browser/platform_util.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/themes/theme_properties.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.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_item.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/download_item_utils.h"
 #include "content/public/browser/download_manager.h"
@@ -61,47 +54,6 @@ int GetOpacity(double animation_progress) {
   return static_cast<int>(255.0 * temp);
 }
 
-void OnGetDownloadDoneForOfflineItem(
-    Profile* profile,
-    base::OnceCallback<void(DownloadUIModelPtr)> callback,
-    const base::Optional<OfflineItem>& offline_item) {
-  if (!offline_item.has_value())
-    return;
-
-  OfflineItemModelManager* manager =
-      OfflineItemModelManagerFactory::GetForBrowserContext(profile);
-  DownloadUIModelPtr model =
-      OfflineItemModel::Wrap(manager, offline_item.value());
-
-  std::move(callback).Run(std::move(model));
-}
-
-void GetDownload(Profile* profile,
-                 ContentId id,
-                 base::OnceCallback<void(DownloadUIModelPtr)> callback) {
-  if (!OfflineItemUtils::IsDownload(id)) {
-    content::DownloadManager* download_manager =
-        content::BrowserContext::GetDownloadManager(profile);
-    if (!download_manager)
-      return;
-
-    DownloadItem* download = download_manager->GetDownloadByGuid(id.id);
-    if (!download)
-      return;
-
-    DownloadUIModelPtr model = DownloadItemModel::Wrap(download);
-    std::move(callback).Run(std::move(model));
-  } else {
-    offline_items_collection::OfflineContentAggregator* aggregator =
-        OfflineContentAggregatorFactory::GetForBrowserContext(profile);
-    if (!aggregator)
-      return;
-
-    aggregator->GetItemById(id, base::BindOnce(&OnGetDownloadDoneForOfflineItem,
-                                               profile, std::move(callback)));
-  }
-}
-
 } // namespace
 
 DownloadShelf::DownloadShelf()
@@ -180,19 +132,19 @@ void DownloadShelf::PaintDownloadInterrupted(
   PaintDownloadComplete(canvas, theme_provider, 1.0 - animation_progress);
 }
 
-void DownloadShelf::AddDownload(DownloadUIModelPtr model) {
-  DCHECK(model);
-  if (model->ShouldRemoveFromShelfWhenComplete()) {
+void DownloadShelf::AddDownload(DownloadItem* download) {
+  DCHECK(download);
+  if (DownloadItemModel(download).ShouldRemoveFromShelfWhenComplete()) {
     // If we are going to remove the download from the shelf upon completion,
     // wait a few seconds to see if it completes quickly. If it's a small
     // download, then the user won't have time to interact with it.
     base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
         FROM_HERE,
         base::BindOnce(&DownloadShelf::ShowDownloadById,
-                       weak_ptr_factory_.GetWeakPtr(), model->GetContentId()),
+                       weak_ptr_factory_.GetWeakPtr(), download->GetId()),
         GetTransientDownloadShowDelay());
   } else {
-    ShowDownload(std::move(model));
+    ShowDownload(download);
   }
 }
 
@@ -236,26 +188,23 @@ base::TimeDelta DownloadShelf::GetTransientDownloadShowDelay() {
   return base::TimeDelta::FromSeconds(kDownloadShowDelayInSeconds);
 }
 
-Profile* DownloadShelf::profile() const {
-  return browser() ? browser()->profile() : nullptr;
+content::DownloadManager* DownloadShelf::GetDownloadManager() {
+  return content::BrowserContext::GetDownloadManager(browser()->profile());
 }
 
-void DownloadShelf::ShowDownload(DownloadUIModelPtr download) {
+void DownloadShelf::ShowDownload(DownloadItem* download) {
   if (download->GetState() == DownloadItem::COMPLETE &&
-      download->ShouldRemoveFromShelfWhenComplete())
+      DownloadItemModel(download).ShouldRemoveFromShelfWhenComplete())
     return;
-
-  if (!DownloadCoreServiceFactory::GetForBrowserContext(download->profile())
+  if (!DownloadCoreServiceFactory::GetForBrowserContext(
+           content::DownloadItemUtils::GetBrowserContext(download))
            ->IsShelfEnabled())
     return;
 
-  bool should_show_download_started_animation =
-      download->ShouldShowDownloadStartedAnimation();
-
   if (is_hidden_)
     Unhide();
   Open();
-  DoAddDownload(std::move(download));
+  DoAddDownload(download);
 
   // browser() can be NULL for tests.
   if (!browser())
@@ -269,15 +218,22 @@ void DownloadShelf::ShowDownload(DownloadUIModelPtr download) {
   // - Rich animations are enabled.
   content::WebContents* shelf_tab =
       browser()->tab_strip_model()->GetActiveWebContents();
-  if (should_show_download_started_animation && shelf_tab &&
+  if (DownloadItemModel(download).ShouldShowDownloadStartedAnimation() &&
+      shelf_tab &&
       platform_util::IsVisible(shelf_tab->GetNativeView()) &&
       gfx::Animation::ShouldRenderRichAnimation()) {
     DownloadStartedAnimation::Show(shelf_tab);
   }
 }
 
-void DownloadShelf::ShowDownloadById(ContentId id) {
-  GetDownload(profile(), id,
-              base::BindOnce(&DownloadShelf::ShowDownload,
-                             weak_ptr_factory_.GetWeakPtr()));
+void DownloadShelf::ShowDownloadById(int32_t download_id) {
+  content::DownloadManager* download_manager = GetDownloadManager();
+  if (!download_manager)
+    return;
+
+  DownloadItem* download = download_manager->GetDownload(download_id);
+  if (!download)
+    return;
+
+  ShowDownload(download);
 }

chrome/browser/download/download_shelf.h

@@ -10,7 +10,6 @@
 #include "base/memory/weak_ptr.h"
 #include "base/time/time.h"
 #include "build/build_config.h"
-#include "chrome/browser/download/download_ui_model.h"
 
 class Browser;
 
@@ -18,14 +17,18 @@ namespace gfx {
 class Canvas;
 }
 
+namespace content {
+class DownloadManager;
+}
+
+namespace download {
+class DownloadItem;
+}
+
 namespace ui {
 class ThemeProvider;
 }
 
-using offline_items_collection::ContentId;
-using offline_items_collection::OfflineItem;
-using DownloadUIModelPtr = DownloadUIModel::DownloadUIModelPtr;
-
 // This is an abstract base class for platform specific download shelf
 // implementations.
 class DownloadShelf {
@@ -83,7 +86,7 @@ class DownloadShelf {
   // DownloadItemModel::ShouldRemoveFromShelfWhenComplete()). These transient
   // downloads are added to the shelf after a delay. If the download completes
   // before the delay duration, it will not be added to the shelf at all.
-  void AddDownload(DownloadUIModelPtr download);
+  void AddDownload(download::DownloadItem* download);
 
   // The browser view needs to know when we are going away to properly return
   // the resize corner size to WebKit so that we don't draw on top of it.
@@ -113,7 +116,7 @@ class DownloadShelf {
   bool is_hidden() { return is_hidden_; }
 
  protected:
-  virtual void DoAddDownload(DownloadUIModelPtr download) = 0;
+  virtual void DoAddDownload(download::DownloadItem* download) = 0;
   virtual void DoOpen() = 0;
   virtual void DoClose(CloseReason reason) = 0;
   virtual void DoHide() = 0;
@@ -123,16 +126,20 @@ class DownloadShelf {
   // Protected virtual for testing.
   virtual base::TimeDelta GetTransientDownloadShowDelay();
 
-  // Virtual for testing.
-  virtual Profile* profile() const;
+  // Returns the DownloadManager associated with this DownloadShelf. All
+  // downloads that are shown on this shelf is expected to belong to this
+  // DownloadManager. Protected virtual for testing.
+  virtual content::DownloadManager* GetDownloadManager();
 
  private:
   // Show the download on the shelf immediately. Also displayes the download
   // started animation if necessary.
-  void ShowDownload(DownloadUIModelPtr download);
+  void ShowDownload(download::DownloadItem* download);
 
-  // Similar to ShowDownload() but refers to the download using an ID.
-  void ShowDownloadById(ContentId id);
+  // Similar to ShowDownload() but refers to the download using an ID. This
+  // download should belong to the DownloadManager returned by
+  // GetDownloadManager().
+  void ShowDownloadById(int32_t download_id);
 
   bool should_show_on_unhide_;
   bool is_hidden_;

chrome/browser/download/download_shelf_controller.cc

@@ -4,28 +4,18 @@
 
 #include "chrome/browser/download/download_shelf_controller.h"
 
-#include "chrome/browser/download/download_shelf.h"
 #include "chrome/browser/download/offline_item_model_manager.h"
 #include "chrome/browser/download/offline_item_model_manager_factory.h"
-#include "chrome/browser/download/offline_item_utils.h"
-#include "chrome/browser/offline_items_collection/offline_content_aggregator_factory.h"
 #include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/ui/browser_window.h"
-#include "components/offline_items_collection/core/offline_content_aggregator.h"
 #include "components/offline_items_collection/core/offline_item.h"
 
 using offline_items_collection::OfflineItemState;
-using offline_items_collection::OfflineContentAggregator;
 
 DownloadShelfController::DownloadShelfController(Profile* profile)
-    : profile_(profile) {
-  aggregator_ = OfflineContentAggregatorFactory::GetForBrowserContext(profile_);
-  aggregator_->AddObserver(this);
-}
+    : profile_(profile) {}
 
-DownloadShelfController::~DownloadShelfController() {
-  aggregator_->RemoveObserver(this);
-}
+DownloadShelfController::~DownloadShelfController() = default;
 
 void DownloadShelfController::OnItemsAdded(
     const OfflineContentProvider::OfflineItemList& items) {
@@ -34,38 +24,28 @@ void DownloadShelfController::OnItemsAdded(
 }
 
 void DownloadShelfController::OnItemRemoved(const ContentId& id) {
-  if (OfflineItemUtils::IsDownload(id))
-    return;
-
   OfflineItemModelManagerFactory::GetForBrowserContext(profile_)
       ->RemoveOfflineItemModelData(id);
 }
 
 void DownloadShelfController::OnItemUpdated(const OfflineItem& item) {
-  if (OfflineItemUtils::IsDownload(item.id))
-    return;
-
   if (item.state == OfflineItemState::CANCELLED)
     return;
 
   OfflineItemModelManager* manager =
       OfflineItemModelManagerFactory::GetForBrowserContext(profile_);
-
-  DownloadUIModel::DownloadUIModelPtr model =
-      OfflineItemModel::Wrap(manager, item);
-
-  if (!model->WasUINotified()) {
-    model->SetWasUINotified(true);
-    OnNewOfflineItemReady(std::move(model));
+  OfflineItemModel model(manager, item);
+  if (!model.WasUINotified()) {
+    model.SetWasUINotified(true);
+    OnNewOfflineItemReady(model);
   }
 }
 
 void DownloadShelfController::OnNewOfflineItemReady(
-    DownloadUIModel::DownloadUIModelPtr model) {
+    const OfflineItemModel& item) {
   Browser* browser = browser = chrome::FindLastActiveWithProfile(profile_);
 
   if (browser && browser->window()) {
     // Add the offline item to DownloadShelf in the browser window.
-    browser->window()->GetDownloadShelf()->AddDownload(std::move(model));
   }
 }

chrome/browser/download/download_shelf_controller.h

@@ -9,15 +9,12 @@
 
 #include "base/macros.h"
 #include "chrome/browser/download/offline_item_model.h"
-#include "components/offline_items_collection/core/offline_content_aggregator.h"
 #include "components/offline_items_collection/core/offline_content_provider.h"
 
 class Profile;
 
 using ContentId = offline_items_collection::ContentId;
 using OfflineContentProvider = offline_items_collection::OfflineContentProvider;
-using OfflineContentAggregator =
-    offline_items_collection::OfflineContentAggregator;
 using OfflineItem = offline_items_collection::OfflineItem;
 
 // Class for notifying UI when an OfflineItem should be displayed.
@@ -34,10 +31,9 @@ class DownloadShelfController : public OfflineContentProvider::Observer {
   void OnItemUpdated(const OfflineItem& item) override;
 
   // Called when a new OfflineItem is to be displayed on UI.
-  void OnNewOfflineItemReady(DownloadUIModel::DownloadUIModelPtr model);
+  void OnNewOfflineItemReady(const OfflineItemModel& item);
 
   Profile* profile_;
-  OfflineContentAggregator* aggregator_;
 
   DISALLOW_COPY_AND_ASSIGN(DownloadShelfController);
 };

chrome/browser/download/download_shelf_unittest.cc

@@ -34,17 +34,13 @@ class DownloadShelfTest : public testing::Test {
   DownloadShelfTest();
 
  protected:
-  DownloadUIModelPtr model() {
-    DownloadUIModelPtr model(
-        new DownloadItemModel(download_item_.get()),
-        base::OnTaskRunnerDeleter(base::ThreadTaskRunnerHandle::Get()));
-    return model;
-  }
-
+  download::MockDownloadItem* download_item() { return download_item_.get(); }
   content::MockDownloadManager* download_manager() {
     return download_manager_.get();
   }
-  TestDownloadShelf* shelf() { return &shelf_; }
+  TestDownloadShelf* shelf() {
+    return &shelf_;
+  }
   Profile* profile() { return profile_.get(); }
 
   void SetUp() override {
@@ -53,7 +49,7 @@ class DownloadShelfTest : public testing::Test {
   void TearDown() override {
   }
 
- protected:
+ private:
   std::unique_ptr<download::MockDownloadItem> GetInProgressMockDownload();
 
   content::TestBrowserThreadBundle test_browser_thread_bundle_;
@@ -65,8 +61,6 @@ class DownloadShelfTest : public testing::Test {
 
 DownloadShelfTest::DownloadShelfTest() : profile_(new TestingProfile()) {
   download_item_.reset(new ::testing::NiceMock<download::MockDownloadItem>());
-  ON_CALL(*download_item_, GetGuid())
-      .WillByDefault(ReturnRefOfCopy(std::string("TEST_GUID")));
   ON_CALL(*download_item_, GetAutoOpened()).WillByDefault(Return(false));
   ON_CALL(*download_item_, GetMimeType()).WillByDefault(Return("text/plain"));
   ON_CALL(*download_item_, GetOpenWhenComplete()).WillByDefault(Return(false));
@@ -84,14 +78,12 @@ DownloadShelfTest::DownloadShelfTest() : profile_(new TestingProfile()) {
 
   download_manager_.reset(
       new ::testing::NiceMock<content::MockDownloadManager>());
-  ON_CALL(*download_manager_, GetDownloadByGuid(_))
+  ON_CALL(*download_manager_, GetDownload(_))
       .WillByDefault(Return(download_item_.get()));
   ON_CALL(*download_manager_, GetBrowserContext())
       .WillByDefault(Return(profile()));
 
-  content::BrowserContext::SetDownloadManagerForTesting(
-      profile_.get(), std::move(download_manager_));
-  shelf_.set_profile(profile_.get());
+  shelf_.set_download_manager(download_manager_.get());
 }
 
 } // namespace
@@ -122,20 +114,20 @@ TEST_F(DownloadShelfTest, UnhideDoesntShowIfNotShownOnHide) {
 TEST_F(DownloadShelfTest, AddDownloadWhileHiddenUnhides) {
   shelf()->Open();
   shelf()->Hide();
-  shelf()->AddDownload(model());
+  shelf()->AddDownload(download_item());
   EXPECT_TRUE(shelf()->IsShowing());
 }
 
 TEST_F(DownloadShelfTest, AddDownloadWhileHiddenUnhidesAndShows) {
   shelf()->Hide();
-  shelf()->AddDownload(model());
+  shelf()->AddDownload(download_item());
   EXPECT_TRUE(shelf()->IsShowing());
 }
 
 // Normal downloads should be added synchronously and cause the shelf to show.
 TEST_F(DownloadShelfTest, AddNormalDownload) {
   EXPECT_FALSE(shelf()->IsShowing());
-  shelf()->AddDownload(model());
+  shelf()->AddDownload(download_item());
   EXPECT_TRUE(shelf()->did_add_download());
   EXPECT_TRUE(shelf()->IsShowing());
 }
@@ -144,35 +136,68 @@ TEST_F(DownloadShelfTest, AddNormalDownload) {
 // should be added after a delay. For testing, the delay is set to 0 seconds. So
 // the download should be added once the message loop is flushed.
 TEST_F(DownloadShelfTest, AddDelayedDownload) {
-  ON_CALL(*download_item_, ShouldOpenFileBasedOnExtension())
-      .WillByDefault(Return(true));
-  ASSERT_TRUE(model()->ShouldRemoveFromShelfWhenComplete());
-  shelf()->AddDownload(model());
+  EXPECT_CALL(*download_item(), ShouldOpenFileBasedOnExtension())
+      .WillRepeatedly(Return(true));
+  ASSERT_TRUE(DownloadItemModel(download_item())
+              .ShouldRemoveFromShelfWhenComplete());
+  shelf()->AddDownload(download_item());
 
   EXPECT_FALSE(shelf()->did_add_download());
   EXPECT_FALSE(shelf()->IsShowing());
 
-  base::RunLoop().RunUntilIdle();
+  base::RunLoop run_loop;
+  run_loop.RunUntilIdle();
+
+  EXPECT_TRUE(shelf()->did_add_download());
+  EXPECT_TRUE(shelf()->IsShowing());
 }
 
 // Add a transient download that completes before the delay. It should not be
 // displayed on the shelf.
 TEST_F(DownloadShelfTest, AddDelayedCompletedDownload) {
-  ON_CALL(*download_item_, ShouldOpenFileBasedOnExtension())
-      .WillByDefault(Return(true));
-  ASSERT_TRUE(model()->ShouldRemoveFromShelfWhenComplete());
-  ON_CALL(*download_item_, IsTemporary()).WillByDefault(Return(true));
-  shelf()->AddDownload(model());
+  EXPECT_CALL(*download_item(), ShouldOpenFileBasedOnExtension())
+      .WillRepeatedly(Return(true));
+  ASSERT_TRUE(DownloadItemModel(download_item())
+              .ShouldRemoveFromShelfWhenComplete());
+  shelf()->AddDownload(download_item());
 
   EXPECT_FALSE(shelf()->did_add_download());
   EXPECT_FALSE(shelf()->IsShowing());
 
-  ON_CALL(*download_item_, GetState())
-      .WillByDefault(Return(DownloadItem::COMPLETE));
-  ON_CALL(*download_item_, GetAutoOpened()).WillByDefault(Return(true));
+  EXPECT_CALL(*download_item(), GetState())
+      .WillRepeatedly(Return(DownloadItem::COMPLETE));
+  EXPECT_CALL(*download_item(), GetAutoOpened())
+      .WillRepeatedly(Return(true));
 
-  base::RunLoop().RunUntilIdle();
+  base::RunLoop run_loop;
+  run_loop.RunUntilIdle();
 
   EXPECT_FALSE(shelf()->did_add_download());
   EXPECT_FALSE(shelf()->IsShowing());
 }
+
+// Add a transient download that completes and becomes non-transient before the
+// delay. It should be displayed on the shelf even though it is complete.
+TEST_F(DownloadShelfTest, AddDelayedCompleteNonTransientDownload) {
+  EXPECT_CALL(*download_item(), ShouldOpenFileBasedOnExtension())
+      .WillRepeatedly(Return(true));
+  ASSERT_TRUE(DownloadItemModel(download_item())
+              .ShouldRemoveFromShelfWhenComplete());
+  shelf()->AddDownload(download_item());
+
+  EXPECT_FALSE(shelf()->did_add_download());
+  EXPECT_FALSE(shelf()->IsShowing());
+
+  EXPECT_CALL(*download_item(), GetState())
+      .WillRepeatedly(Return(DownloadItem::COMPLETE));
+  EXPECT_CALL(*download_item(), ShouldOpenFileBasedOnExtension())
+      .WillRepeatedly(Return(false));
+  ASSERT_FALSE(DownloadItemModel(download_item())
+               .ShouldRemoveFromShelfWhenComplete());
+
+  base::RunLoop run_loop;
+  run_loop.RunUntilIdle();
+
+  EXPECT_TRUE(shelf()->did_add_download());
+  EXPECT_TRUE(shelf()->IsShowing());
+}

chrome/browser/download/download_ui_controller.cc

@@ -98,10 +98,8 @@ void DownloadShelfUIControllerDelegate::OnNewDownloadReady(
 
   if (browser && browser->window() &&
       DownloadItemModel(item).ShouldShowInShelf()) {
-    DownloadUIModel::DownloadUIModelPtr model = DownloadItemModel::Wrap(item);
-
     // GetDownloadShelf creates the download shelf if it was not yet created.
-    browser->window()->GetDownloadShelf()->AddDownload(std::move(model));
+    browser->window()->GetDownloadShelf()->AddDownload(item);
   }
 }
 

chrome/browser/download/offline_item_model.cc

@@ -15,16 +15,6 @@ using offline_items_collection::FailState;
 using offline_items_collection::OfflineItem;
 using offline_items_collection::OfflineItemState;
 
-// static
-DownloadUIModel::DownloadUIModelPtr OfflineItemModel::Wrap(
-    OfflineItemModelManager* manager,
-    const OfflineItem& offline_item) {
-  DownloadUIModel::DownloadUIModelPtr model(
-      new OfflineItemModel(manager, offline_item),
-      base::OnTaskRunnerDeleter(base::ThreadTaskRunnerHandle::Get()));
-  return model;
-}
-
 OfflineItemModel::OfflineItemModel(OfflineItemModelManager* manager,
                                    const OfflineItem& offline_item)
     : manager_(manager),
@@ -42,14 +32,6 @@ OfflineItemModel::~OfflineItemModel() {
     offline_item_observer_->RemoveObserver(offline_item_->id, this);
 }
 
-Profile* OfflineItemModel::profile() const {
-  return Profile::FromBrowserContext(manager_->browser_context());
-}
-
-ContentId OfflineItemModel::GetContentId() const {
-  return offline_item_ ? offline_item_->id : ContentId();
-}
-
 int64_t OfflineItemModel::GetCompletedBytes() const {
   return offline_item_ ? offline_item_->received_bytes : 0;
 }
@@ -84,43 +66,6 @@ base::FilePath OfflineItemModel::GetTargetFilePath() const {
   return offline_item_ ? offline_item_->file_path : base::FilePath();
 }
 
-void OfflineItemModel::OpenDownload() {
-  if (!offline_item_)
-    return;
-
-  GetProvider()->OpenItem(
-      offline_items_collection::LaunchLocation::DOWNLOAD_SHELF,
-      offline_item_->id);
-}
-
-void OfflineItemModel::Pause() {
-  if (!offline_item_)
-    return;
-
-  GetProvider()->PauseDownload(offline_item_->id);
-}
-
-void OfflineItemModel::Resume() {
-  if (!offline_item_)
-    return;
-
-  GetProvider()->ResumeDownload(offline_item_->id, true /* has_user_gesture */);
-}
-
-void OfflineItemModel::Cancel(bool user_cancel) {
-  if (!offline_item_)
-    return;
-
-  GetProvider()->CancelDownload(offline_item_->id);
-}
-
-void OfflineItemModel::Remove() {
-  if (!offline_item_)
-    return;
-
-  GetProvider()->RemoveItem(offline_item_->id);
-}
-
 download::DownloadItem::DownloadState OfflineItemModel::GetState() const {
   if (!offline_item_)
     return download::DownloadItem::CANCELLED;
@@ -146,8 +91,9 @@ download::DownloadItem::DownloadState OfflineItemModel::GetState() const {
 }
 
 bool OfflineItemModel::IsPaused() const {
-  return offline_item_ ? offline_item_->state == OfflineItemState::PAUSED
-                       : true;
+  return offline_item_
+             ? offline_item_->state == offline_items_collection::PAUSED
+             : true;
 }
 
 bool OfflineItemModel::TimeRemaining(base::TimeDelta* remaining) const {
@@ -216,18 +162,6 @@ GURL OfflineItemModel::GetURL() const {
   return offline_item_ ? offline_item_->page_url : GURL();
 }
 
-bool OfflineItemModel::ShouldRemoveFromShelfWhenComplete() const {
-  // TODO(shaktisahu): Add more appropriate logic.
-  return false;
-}
-
-OfflineContentProvider* OfflineItemModel::GetProvider() const {
-  offline_items_collection::OfflineContentAggregator* aggregator =
-      OfflineContentAggregatorFactory::GetForBrowserContext(
-          manager_->browser_context());
-  return aggregator;
-}
-
 void OfflineItemModel::OnItemRemoved(const ContentId& id) {
   for (auto& obs : observers_)
     obs.OnDownloadDestroyed();

chrome/browser/download/offline_item_model.h

@@ -9,42 +9,28 @@
 
 #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_content_provider.h"
 #include "components/offline_items_collection/core/offline_item.h"
 
 class OfflineItemModelManager;
 
 using offline_items_collection::FilteredOfflineItemObserver;
-using offline_items_collection::OfflineContentProvider;
-using offline_items_collection::OfflineItem;
-using offline_items_collection::ContentId;
 
 // Implementation of DownloadUIModel that wrappers around a |OfflineItem|.
 class OfflineItemModel : public DownloadUIModel,
                          public FilteredOfflineItemObserver::Observer {
  public:
-  static DownloadUIModelPtr Wrap(OfflineItemModelManager* manager,
-                                 const OfflineItem& offline_item);
-
   // Constructs a OfflineItemModel.
   OfflineItemModel(OfflineItemModelManager* manager,
-                   const OfflineItem& offline_item);
+                   const offline_items_collection::OfflineItem& offline_item);
   ~OfflineItemModel() override;
 
   // DownloadUIModel implementation.
-  Profile* profile() const override;
-  ContentId GetContentId() const override;
   int64_t GetCompletedBytes() const override;
   int64_t GetTotalBytes() const override;
   int PercentComplete() const override;
   bool WasUINotified() const override;
   void SetWasUINotified(bool should_notify) override;
   base::FilePath GetTargetFilePath() const override;
-  void OpenDownload() override;
-  void Pause() override;
-  void Resume() override;
-  void Cancel(bool user_cancel) override;
-  void Remove() override;
   download::DownloadItem::DownloadState GetState() const override;
   bool IsPaused() const override;
   bool TimeRemaining(base::TimeDelta* remaining) const override;
@@ -55,7 +41,6 @@ class OfflineItemModel : public DownloadUIModel,
   bool AllDataSaved() const override;
   bool GetFileExternallyRemoved() const override;
   GURL GetURL() const override;
-  bool ShouldRemoveFromShelfWhenComplete() const override;
 
 #if !defined(OS_ANDROID)
   bool IsCommandEnabled(const DownloadCommands* download_commands,
@@ -67,11 +52,10 @@ class OfflineItemModel : public DownloadUIModel,
 #endif
 
  private:
-  OfflineContentProvider* GetProvider() const;
-
   // FilteredOfflineItemObserver::Observer overrides.
-  void OnItemRemoved(const ContentId& id) override;
-  void OnItemUpdated(const OfflineItem& item) override;
+  void OnItemRemoved(const offline_items_collection::ContentId& id) override;
+  void OnItemUpdated(
+      const offline_items_collection::OfflineItem& item) override;
 
   // DownloadUIModel implementation.
   std::string GetMimeType() const override;
@@ -79,7 +63,7 @@ class OfflineItemModel : public DownloadUIModel,
   OfflineItemModelManager* manager_;
 
   std::unique_ptr<FilteredOfflineItemObserver> offline_item_observer_;
-  std::unique_ptr<OfflineItem> offline_item_;
+  std::unique_ptr<offline_items_collection::OfflineItem> offline_item_;
 
   DISALLOW_COPY_AND_ASSIGN(OfflineItemModel);
 };

chrome/browser/download/offline_item_model_manager_factory.cc

@@ -6,7 +6,6 @@
 
 #include "base/memory/singleton.h"
 #include "chrome/browser/download/offline_item_model_manager.h"
-#include "chrome/browser/profiles/incognito_helpers.h"
 #include "components/keyed_service/content/browser_context_dependency_manager.h"
 #include "content/public/browser/browser_context.h"
 
@@ -33,8 +32,3 @@ KeyedService* OfflineItemModelManagerFactory::BuildServiceInstanceFor(
     content::BrowserContext* context) const {
   return new OfflineItemModelManager(context);
 }
-
-content::BrowserContext* OfflineItemModelManagerFactory::GetBrowserContextToUse(
-    content::BrowserContext* context) const {
-  return chrome::GetBrowserContextOwnInstanceInIncognito(context);
-}

chrome/browser/download/offline_item_model_manager_factory.h

@@ -42,8 +42,6 @@ class OfflineItemModelManagerFactory
   // BrowserContextKeyedServiceFactory implementation.
   KeyedService* BuildServiceInstanceFor(
       content::BrowserContext* context) const override;
-  content::BrowserContext* GetBrowserContextToUse(
-      content::BrowserContext* context) const override;
 
   DISALLOW_COPY_AND_ASSIGN(OfflineItemModelManagerFactory);
 };

chrome/browser/download/offline_item_utils.cc

@@ -25,9 +25,6 @@ const char kDownloadNamespace[] = "LEGACY_DOWNLOAD";
 // The namespace for incognito downloads.
 const char kDownloadIncognitoNamespace[] = "LEGACY_DOWNLOAD_INCOGNITO";
 
-// Prefix that all download namespaces should start with.
-const char kDownloadNamespacePrefix[] = "LEGACY_DOWNLOAD";
-
 // The remaining time for a download item if it cannot be calculated.
 constexpr int64_t kUnknownRemainingTime = -1;
 
@@ -124,7 +121,3 @@ OfflineItem OfflineItemUtils::CreateOfflineItem(DownloadItem* download_item) {
 std::string OfflineItemUtils::GetDownloadNamespace(bool is_off_the_record) {
   return is_off_the_record ? kDownloadIncognitoNamespace : kDownloadNamespace;
 }
-
-bool OfflineItemUtils::IsDownload(const ContentId& id) {
-  return id.name_space.find(kDownloadNamespacePrefix) != std::string::npos;
-}

chrome/browser/download/offline_item_utils.h

@@ -21,8 +21,6 @@ class OfflineItemUtils {
 
   static std::string GetDownloadNamespace(bool is_off_the_record);
 
-  static bool IsDownload(const offline_items_collection::ContentId& id);
-
  private:
   DISALLOW_COPY_AND_ASSIGN(OfflineItemUtils);
 };

chrome/browser/download/test_download_shelf.cc

@@ -7,9 +7,14 @@
 #include "content/public/browser/download_manager.h"
 
 TestDownloadShelf::TestDownloadShelf()
-    : is_showing_(false), did_add_download_(false), profile_(nullptr) {}
+    : is_showing_(false),
+      did_add_download_(false),
+      download_manager_(NULL) {
+}
 
 TestDownloadShelf::~TestDownloadShelf() {
+  if (download_manager_)
+    download_manager_->RemoveObserver(this);
 }
 
 bool TestDownloadShelf::IsShowing() const {
@@ -24,7 +29,21 @@ Browser* TestDownloadShelf::browser() const {
   return NULL;
 }
 
-void TestDownloadShelf::DoAddDownload(DownloadUIModelPtr download) {
+void TestDownloadShelf::set_download_manager(
+    content::DownloadManager* download_manager) {
+  if (download_manager_)
+    download_manager_->RemoveObserver(this);
+  download_manager_ = download_manager;
+  if (download_manager_)
+    download_manager_->AddObserver(this);
+}
+
+void TestDownloadShelf::ManagerGoingDown(content::DownloadManager* manager) {
+  DCHECK_EQ(manager, download_manager_);
+  download_manager_ = NULL;
+}
+
+void TestDownloadShelf::DoAddDownload(download::DownloadItem* download) {
   did_add_download_ = true;
 }
 
@@ -48,6 +67,6 @@ base::TimeDelta TestDownloadShelf::GetTransientDownloadShowDelay() {
   return base::TimeDelta();
 }
 
-Profile* TestDownloadShelf::profile() const {
-  return profile_;
+content::DownloadManager* TestDownloadShelf::GetDownloadManager() {
+  return download_manager_;
 }

chrome/browser/download/test_download_shelf.h

@@ -12,7 +12,8 @@
 #include "content/public/browser/download_manager.h"
 
 // An implementation of DownloadShelf for testing.
-class TestDownloadShelf : public DownloadShelf {
+class TestDownloadShelf : public DownloadShelf,
+                          public content::DownloadManager::Observer {
  public:
   TestDownloadShelf();
   ~TestDownloadShelf() override;
@@ -25,22 +26,25 @@ class TestDownloadShelf : public DownloadShelf {
   // Return |true| if a download was added to this shelf.
   bool did_add_download() const { return did_add_download_; }
 
-  // Set a profile.
-  void set_profile(Profile* profile) { profile_ = profile; }
+  // Set download_manager_ (and the result of calling GetDownloadManager())
+  void set_download_manager(content::DownloadManager* download_manager);
+
+  // DownloadManager::Observer implementation.
+  void ManagerGoingDown(content::DownloadManager* manager) override;
 
  protected:
-  void DoAddDownload(DownloadUIModelPtr download) override;
+  void DoAddDownload(download::DownloadItem* download) override;
   void DoOpen() override;
   void DoClose(CloseReason reason) override;
   void DoHide() override;
   void DoUnhide() override;
   base::TimeDelta GetTransientDownloadShowDelay() override;
-  Profile* profile() const override;
+  content::DownloadManager* GetDownloadManager() override;
 
  private:
   bool is_showing_;
   bool did_add_download_;
-  Profile* profile_;
+  content::DownloadManager* download_manager_;
 
   DISALLOW_COPY_AND_ASSIGN(TestDownloadShelf);
 };

chrome/browser/offline_pages/android/offline_page_bridge.cc

@@ -221,9 +221,6 @@ void ValidateFileCallback(
       offline_header.reason =
           offline_pages::OfflinePageHeader::Reason::NET_ERROR_SUGGESTION;
       break;
-    case offline_items_collection::LaunchLocation::DOWNLOAD_SHELF:
-      NOTREACHED();
-      break;
   }
   offline_header.need_to_persist = true;
   offline_header.id = base::Int64ToString(offline_id);

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

@@ -119,10 +119,12 @@ void DownloadShelfView::AddDownloadView(DownloadItemView* view) {
   new_item_animation_.Show();
 }
 
-void DownloadShelfView::DoAddDownload(
-    DownloadUIModel::DownloadUIModelPtr download) {
+void DownloadShelfView::DoAddDownload(DownloadItem* download) {
+  DownloadUIModel::DownloadUIModelPtr model(
+      new DownloadItemModel(download),
+      base::OnTaskRunnerDeleter(base::ThreadTaskRunnerHandle::Get()));
   AddDownloadView(
-      new DownloadItemView(std::move(download), this, accessible_alert_));
+      new DownloadItemView(std::move(model), this, accessible_alert_));
 }
 
 void DownloadShelfView::MouseMovedOutOfHost() {

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

@@ -27,6 +27,10 @@ namespace content {
 class PageNavigator;
 }
 
+namespace download {
+class DownloadItem;
+}
+
 namespace views {
 class ImageButton;
 class MdTextButton;
@@ -92,7 +96,7 @@ class DownloadShelfView : public views::AccessiblePaneView,
 
  protected:
   // DownloadShelf:
-  void DoAddDownload(DownloadUIModel::DownloadUIModelPtr download) override;
+  void DoAddDownload(download::DownloadItem* download) override;
   void DoOpen() override;
   void DoClose(CloseReason reason) override;
   void DoHide() override;

components/offline_items_collection/core/launch_location.h

@@ -23,8 +23,6 @@ enum class LaunchLocation {
   SUGGESTION,
   // Due to clicking a suggestion on the net error page.
   NET_ERROR_SUGGESTION,
-  // From Download shelf.
-  DOWNLOAD_SHELF,
 };
 
 }  // namespace offline_items_collection