downloads: add the ability to undo download removal.

chrome/browser/ui/webui/downloads_dom_handler.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/webui/downloads_dom_handler.h"
 
 #include <algorithm>
 #include <functional>
 
 #include "base/basictypes.h"
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/i18n/rtl.h"
 #include "base/i18n/time_formatting.h"
 #include "base/memory/singleton.h"
 #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"
 #include "base/value_conversions.h"
 #include "base/values.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/download/download_crx_util.h"
 #include "chrome/browser/download/download_danger_prompt.h"
 #include "chrome/browser/download/download_history.h"
 #include "chrome/browser/download/download_item_model.h"
@@ skipping 95 matching lines @@
   // any keys in file_value.
   base::DictionaryValue* file_value = new base::DictionaryValue();
 
   file_value->SetInteger(
       "started", static_cast<int>(download_item->GetStartTime().ToTimeT()));
   file_value->SetString(
       "since_string", ui::TimeFormat::RelativeDate(
           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));
   file_value->SetString("file_url",
                         net::FilePathToFileURL(download_path).spec());
 
   extensions::DownloadedByExtension* by_ext =
       extensions::DownloadedByExtension::Get(download_item);
   if (by_ext) {
     file_value->SetString("by_ext_id", by_ext->id());
     file_value->SetString("by_ext_name", by_ext->name());
     // Lookup the extension's current name() in case the user changed their
     // language. This won't work if the extension was uninstalled, so the name
     // might be the wrong language.
     bool include_disabled = true;
     const extensions::Extension* extension = extensions::ExtensionSystem::Get(
         Profile::FromBrowserContext(download_item->GetBrowserContext()))->
       extension_service()->GetExtensionById(by_ext->id(), include_disabled);
     if (extension)
       file_value->SetString("by_ext_name", extension->name());
   }
 
   // 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());
   file_value->SetBoolean("otr", incognito);
   file_value->SetInteger("total", static_cast<int>(
       download_item->GetTotalBytes()));
   file_value->SetBoolean("file_externally_removed",
                          download_item->GetFileExternallyRemoved());
   file_value->SetBoolean("retry", false); // Overridden below if needed.
   file_value->SetBoolean("resume", download_item->CanResume());
@@ skipping 52 matching lines @@
     case content::DownloadItem::CANCELLED:
       file_value->SetString("state", "CANCELLED");
       file_value->SetBoolean("retry", true);
       break;
 
     case content::DownloadItem::COMPLETE:
       DCHECK(!download_item->IsDangerous());
       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;
 }
 
 // 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.GetState() != content::DownloadItem::REMOVED &&
           !item.GetFileNameToReportUser().empty() &&
           !item.GetTargetFilePath().empty());
 }
 
 }  // namespace
 
 DownloadsDOMHandler::DownloadsDOMHandler(content::DownloadManager* dlm)
     : main_notifier_(dlm, this),
       update_scheduled_(false),
       weak_ptr_factory_(this) {
   // Create our fileicon data source.
   Profile* profile = Profile::FromBrowserContext(dlm->GetBrowserContext());
   content::URLDataSource::Add(profile, new FileIconSource());
 
   if (profile->IsOffTheRecord()) {
     original_notifier_.reset(new AllDownloadItemNotifier(
         BrowserContext::GetDownloadManager(profile->GetOriginalProfile()),
         this));
   }
 }
 
 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: -----------------------------------------------
 
 void DownloadsDOMHandler::OnPageLoaded(const base::ListValue* args) {
   SendCurrentDownloads();
 }
 
 void DownloadsDOMHandler::RegisterMessages() {
   web_ui()->RegisterMessageCallback("onPageLoaded",
@@ skipping 28 matching lines @@
                  weak_ptr_factory_.GetWeakPtr()));
   web_ui()->RegisterMessageCallback("cancel",
       base::Bind(&DownloadsDOMHandler::HandleCancel,
                  weak_ptr_factory_.GetWeakPtr()));
   web_ui()->RegisterMessageCallback("clearAll",
       base::Bind(&DownloadsDOMHandler::HandleClearAll,
                  weak_ptr_factory_.GetWeakPtr()));
   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(
     content::DownloadManager* manager, content::DownloadItem* download_item) {
   if (IsDownloadDisplayable(*download_item))
     ScheduleSendCurrentDownloads();
 }
 
 void DownloadsDOMHandler::OnDownloadUpdated(
     content::DownloadManager* manager,
@@ skipping 10 matching lines @@
       query.Search(all_items.begin(), all_items.end(), &filtered_items);
       if (filtered_items.empty())
         return;
     }
     base::ListValue results_value;
     results_value.Append(CreateDownloadItemValue(
         download_item,
         (original_notifier_.get() &&
           (manager == main_notifier_.GetManager()))));
     CallDownloadUpdated(results_value);
+  } else if (download_item->GetState() == content::DownloadItem::REMOVED) {
+    ScheduleSendCurrentDownloads();
   }
 }
 
 void DownloadsDOMHandler::OnDownloadRemoved(
     content::DownloadManager* manager,
     content::DownloadItem* download_item) {
   // This relies on |download_item| being removed from DownloadManager in this
   // MessageLoop iteration. |download_item| may not have been removed from
   // DownloadManager when OnDownloadRemoved() is fired, so bounce off the
   // MessageLoop to give it a chance to be removed. SendCurrentDownloads() looks
@@ skipping 78 matching lines @@
   if (file)
     file->Resume();
 }
 
 void DownloadsDOMHandler::HandleRemove(const base::ListValue* args) {
   if (!IsDeletingHistoryAllowed())
     return;
 
   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) {
   CountDownloadsDOMEvents(DOWNLOADS_DOM_EVENT_CANCEL);
   content::DownloadItem* file = GetDownloadByValue(args);
   if (file)
     file->Cancel(true);
 }
 
 void DownloadsDOMHandler::HandleClearAll(const base::ListValue* args) {
@@ skipping 20 matching lines @@
     const base::ListValue* args) {
   CountDownloadsDOMEvents(DOWNLOADS_DOM_EVENT_OPEN_FOLDER);
   content::DownloadManager* manager = main_notifier_.GetManager();
   if (manager) {
     platform_util::OpenItem(
         Profile::FromBrowserContext(manager->GetBrowserContext()),
         DownloadPrefs::FromDownloadManager(manager)->DownloadPath());
   }
 }
 
+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() {
   // Don't call SendCurrentDownloads() every time anything changes. Batch them
   // together instead. This may handle hundreds of OnDownloadDestroyed() calls
   // in a single UI message loop iteration when the user Clears All downloads.
   if (update_scheduled_)
     return;
   update_scheduled_ = true;
   BrowserThread::PostTask(
@@ skipping 63 matching lines @@
 
 bool DownloadsDOMHandler::IsDeletingHistoryAllowed() {
   content::DownloadManager* manager = main_notifier_.GetManager();
   return (manager &&
           Profile::FromBrowserContext(manager->GetBrowserContext())->
               GetPrefs()->GetBoolean(prefs::kAllowDeletingBrowserHistory));
 }
 
 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);
   if (!item && original_notifier_.get() && original_notifier_->GetManager())
     item = original_notifier_->GetManager()->GetDownload(download_id);
   return item;
 }
 
 content::WebContents* DownloadsDOMHandler::GetWebUIWebContents() {
   return web_ui()->GetWebContents();
 }
 
 void DownloadsDOMHandler::CallDownloadsList(const base::ListValue& downloads) {
   web_ui()->CallJavascriptFunction("downloadsList", downloads);
 }
 
 void DownloadsDOMHandler::CallDownloadUpdated(
     const base::ListValue& download_item) {
   web_ui()->CallJavascriptFunction("downloadUpdated", download_item);
 }