downloads: add the ability to undo download removal.

chrome/browser/ui/webui/downloads_dom_handler.cc

Index: chrome/browser/ui/webui/downloads_dom_handler.cc
diff --git a/chrome/browser/ui/webui/downloads_dom_handler.cc b/chrome/browser/ui/webui/downloads_dom_handler.cc
index f347ba33b455b8cfb0f31555b804c19ea5752ca5..2ee32bf88ae1ac8cf25f9d89b8a3e2647c4003a9 100644
--- a/chrome/browser/ui/webui/downloads_dom_handler.cc
+++ b/chrome/browser/ui/webui/downloads_dom_handler.cc
@@ -16,6 +16,7 @@
 #include "base/metrics/field_trial.h"
 #include "base/metrics/histogram.h"
 #include "base/prefs/pref_service.h"
+#include "base/strings/string_number_conversions.h"
 #include "base/strings/string_piece.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/threading/thread.h"
@@ -131,7 +132,7 @@ base::DictionaryValue* CreateDownloadItemValue(
           download_item->GetStartTime(), NULL));
   file_value->SetString(
       "date_string", base::TimeFormatShortDate(download_item->GetStartTime()));
-  file_value->SetInteger("id", download_item->GetId());
+  file_value->SetString("id", base::Uint64ToString(download_item->GetId()));

[arv (Not doing code reviews), 2014/11/13 19:02:30]

JS numbers cannot represent Uint64. This might lead to trouble down in the
future.

[Dan Beam, 2014/11/19 23:09:32]

the JS only gets a string (now)

 
   base::FilePath download_path(download_item->GetTargetFilePath());
   file_value->Set("file_path", base::CreateFilePathValue(download_path));
@@ -156,7 +157,7 @@ base::DictionaryValue* CreateDownloadItemValue(
 
   // Keep file names as LTR.
   base::string16 file_name =
-    download_item->GetFileNameToReportUser().LossyDisplayName();
+      download_item->GetFileNameToReportUser().LossyDisplayName();
   file_name = base::i18n::GetDisplayStringInLTRDirectionality(file_name);
   file_value->SetString("file_name", file_name);
   file_value->SetString("url", download_item->GetURL().spec());
@@ -229,8 +230,9 @@ base::DictionaryValue* CreateDownloadItemValue(
       file_value->SetString("state", "COMPLETE");
       break;
 
+    case content::DownloadItem::REMOVED:

[benjhayden, 2014/11/13 18:01:59]

How do you ensure that this isn't reached?

     case content::DownloadItem::MAX_DOWNLOAD_STATE:
-      NOTREACHED() << "state undefined";
+      NOTREACHED();
   }
 
   return file_value;
@@ -240,6 +242,7 @@ base::DictionaryValue* CreateDownloadItemValue(
 bool IsDownloadDisplayable(const content::DownloadItem& item) {
   return (!download_crx_util::IsExtensionDownload(item) &&
           !item.IsTemporary() &&
+          item.GetState() != content::DownloadItem::REMOVED &&
           !item.GetFileNameToReportUser().empty() &&
           !item.GetTargetFilePath().empty());
 }
@@ -262,6 +265,14 @@ DownloadsDOMHandler::DownloadsDOMHandler(content::DownloadManager* dlm)
 }
 
 DownloadsDOMHandler::~DownloadsDOMHandler() {
+  while (!removed_ids_.empty()) {

[benjhayden, 2014/11/13 18:01:59]

Could you use DownloadQuery to find downloads whose state is removed instead of
using removed_ids_? It might be slightly slower, but it would make it so that
the information about which items are removed is in one place rather than two,
which, besides being DRY, allows for the possibility that undoable remove could
be exposed to extensions.

[Dan Beam, 2014/11/19 23:09:32]

no because that'd give us all removed downloads, not those specific to this
handler/page

[Dan Beam, 2014/11/19 23:14:06]

whoops, meant to delete this -- the code (in general) has changed a lot so ptal
at the updated patchset.

+    uint32 remove_id = removed_ids_.back();
+    removed_ids_.pop_back();
+
+    content::DownloadItem* download = GetDownloadById(remove_id);
+    if (download)
+      download->Remove();
+  }
 }
 
 // DownloadsDOMHandler, public: -----------------------------------------------
@@ -310,6 +321,9 @@ void DownloadsDOMHandler::RegisterMessages() {
   web_ui()->RegisterMessageCallback("openDownloadsFolder",
       base::Bind(&DownloadsDOMHandler::HandleOpenDownloadsFolder,
                  weak_ptr_factory_.GetWeakPtr()));
+  web_ui()->RegisterMessageCallback("undoRemove",
+      base::Bind(&DownloadsDOMHandler::HandleUndoRemove,
+                 weak_ptr_factory_.GetWeakPtr()));
 }
 
 void DownloadsDOMHandler::OnDownloadCreated(
@@ -340,6 +354,8 @@ void DownloadsDOMHandler::OnDownloadUpdated(
         (original_notifier_.get() &&
           (manager == main_notifier_.GetManager()))));
     CallDownloadUpdated(results_value);
+  } else if (download_item->GetState() == content::DownloadItem::REMOVED) {
+    ScheduleSendCurrentDownloads();
   }
 }
 
@@ -438,8 +454,10 @@ void DownloadsDOMHandler::HandleRemove(const base::ListValue* args) {
 
   CountDownloadsDOMEvents(DOWNLOADS_DOM_EVENT_REMOVE);
   content::DownloadItem* file = GetDownloadByValue(args);
-  if (file)
-    file->Remove();
+  if (file) {
+    removed_ids_.push_back(file->GetId());
+    file->MarkRemoved();
+  }
 }
 
 void DownloadsDOMHandler::HandleCancel(const base::ListValue* args) {
@@ -480,6 +498,18 @@ void DownloadsDOMHandler::HandleOpenDownloadsFolder(
   }
 }
 
+void DownloadsDOMHandler::HandleUndoRemove(const base::ListValue* args) {

[arv (Not doing code reviews), 2014/11/13 19:02:30]

Don't we want to send an ID of some kind. My concern is with having multiple
download managers open at the same time.

+  if (removed_ids_.empty())
+    return;
+
+  uint32 last_removed = removed_ids_.back();
+  removed_ids_.pop_back();
+
+  content::DownloadItem* item = GetDownloadById(last_removed);
+  if (item)
+    item->UndoRemove();
+}
+
 // DownloadsDOMHandler, private: ----------------------------------------------
 
 void DownloadsDOMHandler::ScheduleSendCurrentDownloads() {
@@ -563,9 +593,23 @@ bool DownloadsDOMHandler::IsDeletingHistoryAllowed() {
 
 content::DownloadItem* DownloadsDOMHandler::GetDownloadByValue(
     const base::ListValue* args) {
-  int download_id = -1;
-  if (!ExtractIntegerValue(args, &download_id))
+  std::string download_id;
+  if (!args->GetString(0, &download_id)) {
+    NOTREACHED();
+    return NULL;
+  }
+
+  uint64 id;
+  if (!base::StringToUint64(download_id, &id)) {
+    NOTREACHED();
     return NULL;
+  }
+
+  return GetDownloadById(id);
+}
+
+content::DownloadItem* DownloadsDOMHandler::GetDownloadById(
+    uint32 download_id) {
   content::DownloadItem* item = NULL;
   if (main_notifier_.GetManager())
     item = main_notifier_.GetManager()->GetDownload(download_id);