downloads: clicking "remove" on chrome://downloads should hide the shelf item.

R=asanka@chromium.org
BUG=442666

Review URL: https://codereview.chromium.org/966983002

Cr-Commit-Position: refs/heads/master@{#318788}

chrome/browser/download/download_ui_controller.cc

@@ -7,7 +7,6 @@
 #include "base/callback.h"
 #include "base/stl_util.h"
 #include "chrome/browser/download/download_item_model.h"
-#include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/ui/browser_tabstrip.h"
 #include "content/public/browser/download_item.h"
 #include "content/public/browser/web_contents.h"
@@ -17,6 +16,8 @@
 #include "content/public/browser/android/download_controller_android.h"
 #else
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/ui/browser_finder.h"
+#include "chrome/browser/ui/host_desktop.h"
 #endif
 
 namespace {

chrome/browser/ui/browser.cc

@@ -1339,8 +1339,7 @@ void Browser::ShowDownload(content::DownloadItem* download) {
     return;
 
   // GetDownloadShelf creates the download shelf if it was not yet created.
-  DownloadShelf* shelf = window()->GetDownloadShelf();
-  shelf->AddDownload(download);
+  window()->GetDownloadShelf()->AddDownload(download);
 }
 
 ///////////////////////////////////////////////////////////////////////////////

chrome/browser/ui/cocoa/download/download_item_mac.mm

@@ -30,6 +30,11 @@ DownloadItemMac::~DownloadItemMac() {
 void DownloadItemMac::OnDownloadUpdated(content::DownloadItem* download) {
   DCHECK_EQ(download, download_model_.download());
 
+  if (!download_model_.ShouldShowInShelf()) {
+    [item_controller_ remove];  // We're deleted now!
+    return;
+  }
+
   if ([item_controller_ isDangerousMode] && !download_model_.IsDangerous()) {
     // We have been approved.
     [item_controller_ clearDangerousMode];

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

@@ -274,6 +274,11 @@ void DownloadItemView::OnExtractIconComplete(gfx::Image* icon_bitmap) {
 void DownloadItemView::OnDownloadUpdated(DownloadItem* download_item) {
   DCHECK_EQ(download(), download_item);
 
+  if (!model_.ShouldShowInShelf()) {
+    shelf_->RemoveDownloadView(this);  // This will delete us!
+    return;
+  }
+
   if (IsShowingWarningDialog() && !model_.IsDangerous()) {
     // We have been approved.
     ClearWarningDialog();

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

@@ -127,8 +127,7 @@ void DownloadShelfView::AddDownloadView(DownloadItemView* view) {
 }
 
 void DownloadShelfView::DoAddDownload(DownloadItem* download) {
-  DownloadItemView* view = new DownloadItemView(download, this);
-  AddDownloadView(view);
+  AddDownloadView(new DownloadItemView(download, this));
 }
 
 void DownloadShelfView::MouseMovedOutOfHost() {

chrome/browser/ui/webui/downloads_dom_handler.cc

@@ -77,35 +77,6 @@ enum DownloadsDOMEvent {
   DOWNLOADS_DOM_EVENT_MAX
 };
 
-static const char kKey[] = "DownloadsDOMHandlerData";
-
-class DownloadsDOMHandlerData : public base::SupportsUserData::Data {
- public:
-  static DownloadsDOMHandlerData* Get(content::DownloadItem* item) {
-    return static_cast<DownloadsDOMHandlerData*>(item->GetUserData(kKey));
-  }
-
-  static const DownloadsDOMHandlerData* Get(const content::DownloadItem* item) {
-    return static_cast<DownloadsDOMHandlerData*>(item->GetUserData(kKey));
-  }
-
-  static void Set(content::DownloadItem* item, DownloadsDOMHandlerData* data) {
-    item->SetUserData(kKey, data);
-  }
-
-  static DownloadsDOMHandlerData* Create(content::DownloadItem* item) {
-    DownloadsDOMHandlerData* data = new DownloadsDOMHandlerData;
-    item->SetUserData(kKey, data);
-    return data;
-  }
-
-  void set_is_removed(bool is_removed) { is_removed_ = is_removed; }
-  bool is_removed() const { return is_removed_; }
-
- private:
-  bool is_removed_;
-};
-
 void CountDownloadsDOMEvents(DownloadsDOMEvent event) {
   UMA_HISTOGRAM_ENUMERATION("Download.DOMEvent",
                             event,
@@ -267,18 +238,14 @@ base::DictionaryValue* CreateDownloadItemValue(
   return file_value;
 }
 
-bool IsRemoved(const content::DownloadItem& item) {
-  const DownloadsDOMHandlerData* data = DownloadsDOMHandlerData::Get(&item);
-  return data && data->is_removed();
-}
-
 // Filters out extension downloads and downloads that don't have a filename yet.
 bool IsDownloadDisplayable(const content::DownloadItem& item) {
   return !download_crx_util::IsExtensionDownload(item) &&
          !item.IsTemporary() &&
          !item.GetFileNameToReportUser().empty() &&
          !item.GetTargetFilePath().empty() &&
-         !IsRemoved(item);
+         DownloadItemModel(
+             const_cast<content::DownloadItem*>(&item)).ShouldShowInShelf();
 }
 
 }  // namespace
@@ -299,16 +266,7 @@ DownloadsDOMHandler::DownloadsDOMHandler(content::DownloadManager* dlm)
 }
 
 DownloadsDOMHandler::~DownloadsDOMHandler() {
-  while (!removes_.empty()) {
-    const std::set<uint32> remove = removes_.back();
-    removes_.pop_back();
-
-    for (const auto id : remove) {
-      content::DownloadItem* download = GetDownloadById(id);
-      if (download)
-        download->Remove();
-    }
-  }
+  FinalizeRemovals();
 }
 
 // DownloadsDOMHandler, public: -----------------------------------------------
@@ -398,7 +356,7 @@ void DownloadsDOMHandler::OnDownloadUpdated(
 void DownloadsDOMHandler::OnDownloadRemoved(
     content::DownloadManager* manager,
     content::DownloadItem* download_item) {
-  if (IsRemoved(*download_item))
+  if (!DownloadItemModel(download_item).ShouldShowInShelf())
     return;
 
   // This relies on |download_item| being removed from DownloadManager in this
@@ -503,17 +461,17 @@ void DownloadsDOMHandler::HandleRemove(const base::ListValue* args) {
 
 void DownloadsDOMHandler::HandleUndo(const base::ListValue* args) {
   // TODO(dbeam): handle more than removed downloads someday?
-  if (removes_.empty())
+  if (removals_.empty())
     return;
 
-  const std::set<uint32> last_removed_ids = removes_.back();
-  removes_.pop_back();
+  const std::set<uint32> last_removed_ids = removals_.back();
+  removals_.pop_back();
 
   for (auto id : last_removed_ids) {
     content::DownloadItem* download = GetDownloadById(id);
     if (!download)
       continue;
-    DownloadsDOMHandlerData::Set(download, nullptr);
+    DownloadItemModel(download).SetShouldShowInShelf(true);
     download->UpdateObservers();
   }
 }
@@ -543,16 +501,18 @@ void DownloadsDOMHandler::RemoveDownloads(
     const std::vector<content::DownloadItem*>& to_remove) {
   std::set<uint32> ids;
   for (auto* download : to_remove) {
-    if (IsRemoved(*download) ||
+    DownloadItemModel item_model(download);
+    if (!item_model.ShouldShowInShelf() ||
         download->GetState() == content::DownloadItem::IN_PROGRESS) {
       continue;
     }
 
-    DownloadsDOMHandlerData::Create(download)->set_is_removed(true);
+    item_model.SetShouldShowInShelf(false);
     ids.insert(download->GetId());
     download->UpdateObservers();
   }
-  removes_.push_back(ids);
+  if (!ids.empty())
+    removals_.push_back(ids);
 }
 
 void DownloadsDOMHandler::HandleOpenDownloadsFolder(
@@ -585,6 +545,19 @@ content::DownloadManager* DownloadsDOMHandler::GetMainNotifierManager() {
   return main_notifier_.GetManager();
 }
 
+void DownloadsDOMHandler::FinalizeRemovals() {
+  while (!removals_.empty()) {
+    const std::set<uint32> remove = removals_.back();
+    removals_.pop_back();
+
+    for (const auto id : remove) {
+      content::DownloadItem* download = GetDownloadById(id);
+      if (download)
+        download->Remove();
+    }
+  }
+}
+
 void DownloadsDOMHandler::SendCurrentDownloads() {
   update_scheduled_ = false;
   content::DownloadManager::DownloadVector all_items, filtered_items;
@@ -670,8 +643,8 @@ content::DownloadItem* DownloadsDOMHandler::GetDownloadByValue(
 
 content::DownloadItem* DownloadsDOMHandler::GetDownloadById(uint32 id) {
   content::DownloadItem* item = NULL;
-  if (main_notifier_.GetManager())
-    item = main_notifier_.GetManager()->GetDownload(id);
+  if (GetMainNotifierManager())
+    item = GetMainNotifierManager()->GetDownload(id);
   if (!item && original_notifier_.get() && original_notifier_->GetManager())
     item = original_notifier_->GetManager()->GetDownload(id);
   return item;

chrome/browser/ui/webui/downloads_dom_handler.h

@@ -106,6 +106,9 @@ class DownloadsDOMHandler : public content::WebUIMessageHandler,
   // Protected for testing.
   virtual content::DownloadManager* GetMainNotifierManager();
 
+  // Actually remove downloads with an ID in |removals_|. This cannot be undone.
+  void FinalizeRemovals();
+
  private:
   // Shorthand for |observing_items_|, which tracks all items that this is
   // observing so that RemoveObserver will be called for all of them.
@@ -150,7 +153,7 @@ class DownloadsDOMHandler : public content::WebUIMessageHandler,
   scoped_ptr<AllDownloadItemNotifier> original_notifier_;
 
   // IDs of downloads to remove when this handler gets deleted.
-  std::vector<std::set<uint32>> removes_;
+  std::vector<std::set<uint32>> removals_;
 
   // Whether a call to SendCurrentDownloads() is currently scheduled.
   bool update_scheduled_;

chrome/browser/ui/webui/downloads_dom_handler_browsertest.cc

@@ -7,6 +7,7 @@
 #include "base/json/json_reader.h"
 #include "base/prefs/pref_service.h"
 #include "base/values.h"
+#include "chrome/browser/download/download_item_model.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/webui/downloads_dom_handler.h"
@@ -85,6 +86,8 @@ class MockDownloadsDOMHandler : public DownloadsDOMHandler {
 
   void set_manager(content::DownloadManager* manager) { manager_ = manager; }
 
+  using DownloadsDOMHandler::FinalizeRemovals;
+
  protected:
   content::WebContents* GetWebUIWebContents() override { return NULL; }
 
@@ -247,6 +250,7 @@ IN_PROC_BROWSER_TEST_F(DownloadsDOMHandlerTest, ClearAllSkipsInProgress) {
       testing::SetArgPointee<0>(items));
 
   mock_handler_->HandleClearAll(NULL);
+  EXPECT_TRUE(DownloadItemModel(&item).ShouldShowInShelf());
 }
 
 // Tests that DownloadsDOMHandler detects new downloads and relays them to the
@@ -270,6 +274,34 @@ IN_PROC_BROWSER_TEST_F(DownloadsDOMHandlerTest, DownloadsRelayed) {
   EXPECT_EQ(0, static_cast<int>(mock_handler_->downloads_list()->GetSize()));
 }
 
+// Tests that DownloadsDOMHandler actually calls DownloadItem::Remove() when
+// it's closed (and removals can no longer be undone).
+IN_PROC_BROWSER_TEST_F(DownloadsDOMHandlerTest, RemoveCalledOnPageClose) {
+  content::MockDownloadManager manager;
+  mock_handler_->set_manager(&manager);
+
+  content::MockDownloadItem item;
+  EXPECT_CALL(item, GetId()).WillRepeatedly(testing::Return(1));
+  EXPECT_CALL(item, GetState()).WillRepeatedly(
+      testing::Return(content::DownloadItem::COMPLETE));
+
+  DownloadItemModel model(&item);
+  EXPECT_TRUE(model.ShouldShowInShelf());
+
+  EXPECT_CALL(manager, GetDownload(1)).WillRepeatedly(testing::Return(&item));
+
+  base::ListValue remove;
+  remove.AppendString("1");
+  EXPECT_CALL(item, UpdateObservers()).Times(1);
+  mock_handler_->HandleRemove(&remove);
+  EXPECT_FALSE(model.ShouldShowInShelf());
+
+  EXPECT_CALL(item, Remove()).Times(1);
+  // Call |mock_handler_->FinalizeRemovals()| instead of |mock_handler_.reset()|
+  // because the vtable is affected during destruction and the fake manager
+  // rigging doesn't work.
+  mock_handler_->FinalizeRemovals();
+}
 
 // TODO(benjhayden): Test the extension downloads filter for both
 // mock_handler_.downloads_list() and mock_handler_.download_updated().