Rewrite DownloadsDOMHandler

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 6b331f70a6e6854734f5e067ba568bb1766c349b..e7aa6b320150cb88f51d584937040a202a78213d 100644
--- a/chrome/browser/ui/webui/downloads_dom_handler.cc
+++ b/chrome/browser/ui/webui/downloads_dom_handler.cc
@@ -59,7 +59,7 @@ namespace {
 
 // Maximum number of downloads to show. TODO(glen): Remove this and instead
 // stuff the downloads down the pipe slowly.
-static const int kMaxDownloads = 150;
+static const size_t kMaxDownloads = 150;
 
 // Sort DownloadItems into descending order by their start time.
 class DownloadItemSorter : public std::binary_function<content::DownloadItem*,
@@ -120,7 +120,6 @@ const char* GetDangerTypeString(content::DownloadDangerType danger_type) {
 // "otr" set to |incognito|.
 DictionaryValue* CreateDownloadItemValue(
     content::DownloadItem* download,
-    int id,
     bool incognito) {
   DictionaryValue* file_value = new DictionaryValue();
 
@@ -130,7 +129,7 @@ DictionaryValue* CreateDownloadItemValue(
       "since_string", TimeFormat::RelativeDate(download->GetStartTime(), NULL));
   file_value->SetString(
       "date_string", base::TimeFormatShortDate(download->GetStartTime()));
-  file_value->SetInteger("id", id);
+  file_value->SetInteger("id", download->GetId());
 
   FilePath download_path(download->GetTargetFilePath());
   file_value->Set("file_path", base::CreateFilePathValue(download_path));
@@ -214,160 +213,144 @@ bool IsItemIncognito(
           (manager->GetDownload(download_id) != NULL));
 }
 
+// Filter 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.GetFileNameToReportUser().empty() &&
+          !item.GetTargetFilePath().empty());

[Randy Smith (Not in Mondays), 2012/08/17 17:49:47]

This plus the shift to OnDownloadCreated() means that you're going to have a
window during which the user can cancel downloads that you didn't before.  Are
you reasonably comfortable that the right things will happen in that case?  Any
way to do a test?  (Manual is fine.)

[benjhayden, 2012/08/17 20:54:15]

Tested manually by calling Cancel(true) as soon as an item became displayable.
No crashes, the ui updated just fine, sqlite3 confirms that the items still made
it into the history.

+}
+
 }  // namespace
 
 DownloadsDOMHandler::DownloadsDOMHandler(content::DownloadManager* dlm)
     : search_text_(),
       download_manager_(dlm),
       original_profile_download_manager_(NULL),
-      initialized_(false),
+      update_scheduled_(false),
       ALLOW_THIS_IN_INITIALIZER_LIST(weak_ptr_factory_(this)) {
   // Create our fileicon data source.
   Profile* profile = Profile::FromBrowserContext(dlm->GetBrowserContext());
   ChromeURLDataManager::AddDataSource(profile, new FileIconSource());
 
-  // Figure out our parent DownloadManager, if any.
-  Profile* original_profile = profile->GetOriginalProfile();
-  if (original_profile != profile) {
+  // Observe the DownloadManagers.
+  download_manager_->AddObserver(this);
+  if (profile->IsOffTheRecord()) {
     original_profile_download_manager_ =
-        BrowserContext::GetDownloadManager(original_profile);
+        BrowserContext::GetDownloadManager(profile->GetOriginalProfile());
+    original_profile_download_manager_->AddObserver(this);
+  }
+
+  // Observe all the DownloadItems.
+  content::DownloadManager::DownloadVector downloads;
+  SearchDownloads(&downloads);
+  for (content::DownloadManager::DownloadVector::const_iterator
+           iter = downloads.begin();
+       iter != downloads.end(); ++iter) {
+    (*iter)->AddObserver(this);
+    observing_items_.insert(*iter);
   }
 }
 
 DownloadsDOMHandler::~DownloadsDOMHandler() {
-  ClearDownloadItems();
+  for (DownloadSet::const_iterator it = observing_items_.begin();
+       it != observing_items_.end(); ++it) {
+    (*it)->RemoveObserver(this);
+  }
+  observing_items_.clear();
   download_manager_->RemoveObserver(this);
-  if (original_profile_download_manager_)
+  if (original_profile_download_manager_) {
     original_profile_download_manager_->RemoveObserver(this);
+  }
 }
 
 // DownloadsDOMHandler, public: -----------------------------------------------
 
 void DownloadsDOMHandler::OnPageLoaded(const base::ListValue* args) {
-  if (initialized_)
-    return;
-  initialized_ = true;
-
-  download_manager_->AddObserver(this);
-  if (original_profile_download_manager_)
-    original_profile_download_manager_->AddObserver(this);
+  SendCurrentDownloads();
 }
 
 void DownloadsDOMHandler::RegisterMessages() {
   web_ui()->RegisterMessageCallback("onPageLoaded",
       base::Bind(&DownloadsDOMHandler::OnPageLoaded,
-                 base::Unretained(this)));
+                 weak_ptr_factory_.GetWeakPtr()));
   web_ui()->RegisterMessageCallback("getDownloads",
       base::Bind(&DownloadsDOMHandler::HandleGetDownloads,
-                 base::Unretained(this)));
+                 weak_ptr_factory_.GetWeakPtr()));
   web_ui()->RegisterMessageCallback("openFile",
       base::Bind(&DownloadsDOMHandler::HandleOpenFile,
-                 base::Unretained(this)));
+                 weak_ptr_factory_.GetWeakPtr()));
   web_ui()->RegisterMessageCallback("drag",
       base::Bind(&DownloadsDOMHandler::HandleDrag,
-                 base::Unretained(this)));
+                 weak_ptr_factory_.GetWeakPtr()));
   web_ui()->RegisterMessageCallback("saveDangerous",
       base::Bind(&DownloadsDOMHandler::HandleSaveDangerous,
-                 base::Unretained(this)));
+                 weak_ptr_factory_.GetWeakPtr()));
   web_ui()->RegisterMessageCallback("discardDangerous",
       base::Bind(&DownloadsDOMHandler::HandleDiscardDangerous,
-                 base::Unretained(this)));
+                 weak_ptr_factory_.GetWeakPtr()));
   web_ui()->RegisterMessageCallback("show",
       base::Bind(&DownloadsDOMHandler::HandleShow,
-                 base::Unretained(this)));
+                 weak_ptr_factory_.GetWeakPtr()));
   web_ui()->RegisterMessageCallback("togglepause",
       base::Bind(&DownloadsDOMHandler::HandlePause,
-                 base::Unretained(this)));
+                 weak_ptr_factory_.GetWeakPtr()));
   web_ui()->RegisterMessageCallback("resume",
       base::Bind(&DownloadsDOMHandler::HandlePause,
-                 base::Unretained(this)));
+                 weak_ptr_factory_.GetWeakPtr()));
   web_ui()->RegisterMessageCallback("remove",
       base::Bind(&DownloadsDOMHandler::HandleRemove,
-                 base::Unretained(this)));
+                 weak_ptr_factory_.GetWeakPtr()));
   web_ui()->RegisterMessageCallback("cancel",
       base::Bind(&DownloadsDOMHandler::HandleCancel,
-                 base::Unretained(this)));
+                 weak_ptr_factory_.GetWeakPtr()));
   web_ui()->RegisterMessageCallback("clearAll",
       base::Bind(&DownloadsDOMHandler::HandleClearAll,
-                 base::Unretained(this)));
+                 weak_ptr_factory_.GetWeakPtr()));
   web_ui()->RegisterMessageCallback("openDownloadsFolder",
       base::Bind(&DownloadsDOMHandler::HandleOpenDownloadsFolder,
-                 base::Unretained(this)));
-}
-
-void DownloadsDOMHandler::OnDownloadUpdated(content::DownloadItem* download) {
-  // Get the id for the download. Our downloads are sorted latest to first,
-  // and the id is the index into that list. We should be careful of sync
-  // errors between the UI and the download_items_ list (we may wish to use
-  // something other than 'id').
-  OrderedDownloads::iterator it = std::find(download_items_.begin(),
-                                            download_items_.end(),
-                                            download);
-  if (it == download_items_.end())
-    return;
-
-  const int id = static_cast<int>(it - download_items_.begin());
-
-  ListValue results_value;
-  results_value.Append(CreateDownloadItemValue(download, id, IsItemIncognito(
-      download->GetId(),
-      download_manager_,
-      original_profile_download_manager_)));
-  web_ui()->CallJavascriptFunction("downloadUpdated", results_value);
+                 weak_ptr_factory_.GetWeakPtr()));
 }
 
-void DownloadsDOMHandler::OnDownloadDestroyed(
-    content::DownloadItem* download) {
-  download->RemoveObserver(this);
-  OrderedDownloads::iterator it = std::find(download_items_.begin(),
-                                            download_items_.end(),
-                                            download);
-  if (it != download_items_.end())
-    *it = NULL;
-  // A later ModelChanged() notification will change the WebUI's
-  // view of the downloads list.
-}
-
-// A download has started or been deleted. Query our DownloadManager for the
-// current set of downloads.
-void DownloadsDOMHandler::ModelChanged(content::DownloadManager* manager) {
-  DCHECK(manager == download_manager_ ||
-         manager == original_profile_download_manager_);
-
-  ClearDownloadItems();
-  download_manager_->SearchDownloads(WideToUTF16(search_text_),
-                                     &download_items_);
-  // If we have a parent DownloadManager, let it add its downloads to the
-  // results.
-  if (original_profile_download_manager_) {
-    original_profile_download_manager_->SearchDownloads(
-        WideToUTF16(search_text_), &download_items_);
+void DownloadsDOMHandler::OnDownloadCreated(
+    content::DownloadManager* manager, content::DownloadItem* download_item) {
+  download_item->AddObserver(this);
+  observing_items_.insert(download_item);
+  if (IsDownloadDisplayable(*download_item) &&
+      !update_scheduled_) {
+    // Don't call SendCurrentDownloads() every time anything changes. Batch them
+    // together instead. We may handle hundreds of OnDownloadCreated() calls in
+    // a single UI message loop iteration when the history is loaded.
+    update_scheduled_ = true;
+    BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
+        base::Bind(&DownloadsDOMHandler::SendCurrentDownloads,
+                   weak_ptr_factory_.GetWeakPtr()));
   }
+}
 
-  sort(download_items_.begin(), download_items_.end(), DownloadItemSorter());
-
-  // Remove any extension downloads.
-  for (size_t i = 0; i < download_items_.size();) {
-    if (download_crx_util::IsExtensionDownload(*download_items_[i]))
-      download_items_.erase(download_items_.begin() + i);
-    else
-      i++;
+void DownloadsDOMHandler::OnDownloadUpdated(content::DownloadItem* download) {
+  if (IsDownloadDisplayable(*download)) {
+    base::ListValue results_value;
+    results_value.Append(CreateDownloadItemValue(download, IsItemIncognito(
+        download->GetId(),
+        download_manager_,
+        original_profile_download_manager_)));
+    CallDownloadUpdated(results_value);
   }
+}
 
-  // Add ourself to all download items as an observer.
-  for (OrderedDownloads::iterator it = download_items_.begin();
-       it != download_items_.end(); ++it) {
-    if (static_cast<int>(it - download_items_.begin()) > kMaxDownloads)
-      break;
-
-    // We should never see anything that isn't already in the history.
-    DCHECK(*it);
-    DCHECK((*it)->IsPersisted());
-
-    (*it)->AddObserver(this);
+void DownloadsDOMHandler::OnDownloadDestroyed(
+    content::DownloadItem* download_item) {

[Randy Smith (Not in Mondays), 2012/08/17 17:49:47]

Worth DCHECKing to make sure we have it? 

[benjhayden, 2012/08/17 20:54:15]

Who would call OnDownloadDestroyed besides ~DownloadItem?

+  download_item->RemoveObserver(this);
+  observing_items_.erase(download_item);
+  // Don't call SendCurrentDownloads() every time anything changes. Batch them
+  // together instead. We may handle hundreds of OnDownloadDestroyed() calls in
+  // a single UI message loop iteration when the user Clears All downloads.
+  if (!update_scheduled_) {
+    update_scheduled_ = true;
+    BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
+        base::Bind(&DownloadsDOMHandler::SendCurrentDownloads,
+                   weak_ptr_factory_.GetWeakPtr()));
   }
-
-  SendCurrentDownloads();
 }
 
 void DownloadsDOMHandler::ManagerGoingDown(content::DownloadManager* manager) {
@@ -380,73 +363,65 @@ void DownloadsDOMHandler::ManagerGoingDown(content::DownloadManager* manager) {
   NOTREACHED();
 }
 
-void DownloadsDOMHandler::HandleGetDownloads(const ListValue* args) {
+void DownloadsDOMHandler::HandleGetDownloads(const base::ListValue* args) {
   CountDownloadsDOMEvents(DOWNLOADS_DOM_EVENT_GET_DOWNLOADS);
-  std::wstring new_search = UTF16ToWideHack(ExtractStringValue(args));
-  if (search_text_.compare(new_search) != 0) {
-    search_text_ = new_search;
-    ModelChanged(download_manager_);
-  } else {
-    SendCurrentDownloads();
-  }
-
+  search_text_ = ExtractStringValue(args);
+  SendCurrentDownloads();
   download_manager_->CheckForHistoryFilesRemoval();
 }
 
-void DownloadsDOMHandler::HandleOpenFile(const ListValue* args) {
+void DownloadsDOMHandler::HandleOpenFile(const base::ListValue* args) {
   CountDownloadsDOMEvents(DOWNLOADS_DOM_EVENT_OPEN_FILE);
   content::DownloadItem* file = GetDownloadByValue(args);
   if (file)
     file->OpenDownload();
 }
 
-void DownloadsDOMHandler::HandleDrag(const ListValue* args) {
+void DownloadsDOMHandler::HandleDrag(const base::ListValue* args) {
   CountDownloadsDOMEvents(DOWNLOADS_DOM_EVENT_DRAG);
   content::DownloadItem* file = GetDownloadByValue(args);
-  if (file) {
-    IconManager* im = g_browser_process->icon_manager();
-    gfx::Image* icon = im->LookupIcon(file->GetUserVerifiedFilePath(),
-                                      IconLoader::NORMAL);
-    gfx::NativeView view = web_ui()->GetWebContents()->GetNativeView();
-    {
-      // Enable nested tasks during DnD, while |DragDownload()| blocks.
-      MessageLoop::ScopedNestableTaskAllower allow(MessageLoop::current());
-      download_util::DragDownload(file, icon, view);
-    }
+  content::WebContents* web_contents = GetWebUIWebContents();
+  if (!file || !web_contents)
+    return;
+  gfx::Image* icon = g_browser_process->icon_manager()->LookupIcon(
+      file->GetUserVerifiedFilePath(), IconLoader::NORMAL);
+  gfx::NativeView view = web_contents->GetNativeView();
+  {
+    // Enable nested tasks during DnD, while |DragDownload()| blocks.
+    MessageLoop::ScopedNestableTaskAllower allow(MessageLoop::current());
+    download_util::DragDownload(file, icon, view);
   }
 }
 
-void DownloadsDOMHandler::HandleSaveDangerous(const ListValue* args) {
+void DownloadsDOMHandler::HandleSaveDangerous(const base::ListValue* args) {
   CountDownloadsDOMEvents(DOWNLOADS_DOM_EVENT_SAVE_DANGEROUS);
   content::DownloadItem* file = GetDownloadByValue(args);
   if (file)
     ShowDangerPrompt(file);
-  // TODO(benjhayden): else ModelChanged()? Downloads might be able to disappear
-  // out from under us, so update our idea of the downloads as soon as possible.
 }
 
-void DownloadsDOMHandler::HandleDiscardDangerous(const ListValue* args) {
+void DownloadsDOMHandler::HandleDiscardDangerous(const base::ListValue* args) {
   CountDownloadsDOMEvents(DOWNLOADS_DOM_EVENT_DISCARD_DANGEROUS);
   content::DownloadItem* file = GetDownloadByValue(args);
   if (file)
     file->Delete(content::DownloadItem::DELETE_DUE_TO_USER_DISCARD);
 }
 
-void DownloadsDOMHandler::HandleShow(const ListValue* args) {
+void DownloadsDOMHandler::HandleShow(const base::ListValue* args) {
   CountDownloadsDOMEvents(DOWNLOADS_DOM_EVENT_SHOW);
   content::DownloadItem* file = GetDownloadByValue(args);
   if (file)
     file->ShowDownloadInShell();
 }
 
-void DownloadsDOMHandler::HandlePause(const ListValue* args) {
+void DownloadsDOMHandler::HandlePause(const base::ListValue* args) {
   CountDownloadsDOMEvents(DOWNLOADS_DOM_EVENT_PAUSE);
   content::DownloadItem* file = GetDownloadByValue(args);
   if (file)
     file->TogglePause();
 }
 
-void DownloadsDOMHandler::HandleRemove(const ListValue* args) {
+void DownloadsDOMHandler::HandleRemove(const base::ListValue* args) {
   CountDownloadsDOMEvents(DOWNLOADS_DOM_EVENT_REMOVE);
   content::DownloadItem* file = GetDownloadByValue(args);
   if (file) {
@@ -455,14 +430,14 @@ void DownloadsDOMHandler::HandleRemove(const ListValue* args) {
   }
 }
 
-void DownloadsDOMHandler::HandleCancel(const ListValue* args) {
+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 ListValue* args) {
+void DownloadsDOMHandler::HandleClearAll(const base::ListValue* args) {
   CountDownloadsDOMEvents(DOWNLOADS_DOM_EVENT_CLEAR_ALL);
   download_manager_->RemoveAllDownloads();
 
@@ -472,7 +447,8 @@ void DownloadsDOMHandler::HandleClearAll(const ListValue* args) {
     original_profile_download_manager_->RemoveAllDownloads();
 }
 
-void DownloadsDOMHandler::HandleOpenDownloadsFolder(const ListValue* args) {
+void DownloadsDOMHandler::HandleOpenDownloadsFolder(
+    const base::ListValue* args) {
   CountDownloadsDOMEvents(DOWNLOADS_DOM_EVENT_OPEN_FOLDER);
   platform_util::OpenItem(
       DownloadPrefs::FromDownloadManager(download_manager_)->DownloadPath());
@@ -481,27 +457,40 @@ void DownloadsDOMHandler::HandleOpenDownloadsFolder(const ListValue* args) {
 // DownloadsDOMHandler, private: ----------------------------------------------
 
 void DownloadsDOMHandler::SendCurrentDownloads() {
-  ListValue results_value;
-  for (OrderedDownloads::iterator it = download_items_.begin();
-      it != download_items_.end(); ++it) {
-    if (!*it)
-      continue;
-    int index = static_cast<int>(it - download_items_.begin());
-    if (index <= kMaxDownloads)
-      results_value.Append(CreateDownloadItemValue(*it, index, IsItemIncognito(
-            (*it)->GetId(),
+  update_scheduled_ = false;
+  content::DownloadManager::DownloadVector downloads;
+  SearchDownloads(&downloads);
+  sort(downloads.begin(), downloads.end(), DownloadItemSorter());
+  base::ListValue results_value;
+  for (content::DownloadManager::DownloadVector::const_iterator
+           iter = downloads.begin();
+       iter != downloads.end(); ++iter) {
+    if (IsDownloadDisplayable(**iter)) {
+      results_value.Append(CreateDownloadItemValue(*iter, IsItemIncognito(
+            (*iter)->GetId(),
             download_manager_,
             original_profile_download_manager_)));
+    }
+    if (results_value.GetSize() == kMaxDownloads)
+      break;
   }
+  CallDownloadsList(results_value);
+}
 
-  web_ui()->CallJavascriptFunction("downloadsList", results_value);
+void DownloadsDOMHandler::SearchDownloads(
+    content::DownloadManager::DownloadVector* downloads) {
+  download_manager_->SearchDownloads(search_text_, downloads);
+  if (original_profile_download_manager_) {
+    original_profile_download_manager_->SearchDownloads(
+        search_text_, downloads);
+  }
 }
 
 void DownloadsDOMHandler::ShowDangerPrompt(
     content::DownloadItem* dangerous_item) {
   DownloadDangerPrompt* danger_prompt = DownloadDangerPrompt::Create(
       dangerous_item,
-      TabContents::FromWebContents(web_ui()->GetWebContents()),
+      TabContents::FromWebContents(GetWebUIWebContents()),
       base::Bind(&DownloadsDOMHandler::DangerPromptAccepted,
                  weak_ptr_factory_.GetWeakPtr(), dangerous_item->GetId()),
       base::Closure());
@@ -518,33 +507,26 @@ void DownloadsDOMHandler::DangerPromptAccepted(int download_id) {
   item->DangerousDownloadValidated();
 }
 
-void DownloadsDOMHandler::ClearDownloadItems() {
-  // Clear out old state and remove self as observer for each download.
-  for (OrderedDownloads::iterator it = download_items_.begin();
-      it != download_items_.end(); ++it) {
-    if (!*it)
-      continue;
-    (*it)->RemoveObserver(this);
+content::DownloadItem* DownloadsDOMHandler::GetDownloadByValue(
+    const base::ListValue* args) {
+  int id = -1;
+  if (!ExtractIntegerValue(args, &id))
+    return NULL;
+  content::DownloadItem* download = download_manager_->GetDownload(id);
+  if (download == NULL) {
+    download = original_profile_download_manager_->GetDownload(id);
   }
-  download_items_.clear();
+  return download;
 }
 
-content::DownloadItem* DownloadsDOMHandler::GetDownloadById(int id) {
-  for (OrderedDownloads::iterator it = download_items_.begin();
-      it != download_items_.end(); ++it) {
-    if (static_cast<int>(it - download_items_.begin() == id)) {
-      return (*it);
-    }
-  }
+content::WebContents* DownloadsDOMHandler::GetWebUIWebContents() {
+  return web_ui()->GetWebContents();
+}
 
-  return NULL;
+void DownloadsDOMHandler::CallDownloadsList(const base::ListValue& downloads) {
+  web_ui()->CallJavascriptFunction("downloadsList", downloads);
 }
 
-content::DownloadItem* DownloadsDOMHandler::GetDownloadByValue(
-    const ListValue* args) {
-  int id;
-  if (ExtractIntegerValue(args, &id)) {
-    return GetDownloadById(id);
-  }
-  return NULL;
+void DownloadsDOMHandler::CallDownloadUpdated(const base::ListValue& download) {
+  web_ui()->CallJavascriptFunction("downloadUpdated", download);
 }