Rewrite DownloadsDOMHandler

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"
@@ skipping 41 matching lines @@
 #endif
 
 using content::BrowserContext;
 using content::BrowserThread;
 using content::UserMetricsAction;
 
 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*,
                                                        content::DownloadItem*,
                                                        bool> {
  public:
   bool operator()(const content::DownloadItem* lhs,
                   const content::DownloadItem* rhs) {
     return lhs->GetStartTime() > rhs->GetStartTime();
   }
@@ skipping 14 matching lines @@
   DOWNLOADS_DOM_EVENT_MAX
 };
 
 void CountDownloadsDOMEvents(DownloadsDOMEvent event) {
   UMA_HISTOGRAM_ENUMERATION("Download.DOMEvent",
                             event,
                             DOWNLOADS_DOM_EVENT_MAX);
 }
 
 // Returns a string constant to be used as the |danger_type| value in
-// CreateDownloadItemValue().  We only return strings for DANGEROUS_FILE,
+// CreateDownloadItemValue().  Only return strings for DANGEROUS_FILE,
 // DANGEROUS_URL, DANGEROUS_CONTENT, and UNCOMMON_CONTENT because the
 // |danger_type| value is only defined if the value of |state| is |DANGEROUS|.
 const char* GetDangerTypeString(content::DownloadDangerType danger_type) {
   switch (danger_type) {
     case content::DOWNLOAD_DANGER_TYPE_DANGEROUS_FILE:
       return "DANGEROUS_FILE";
     case content::DOWNLOAD_DANGER_TYPE_DANGEROUS_URL:
       return "DANGEROUS_URL";
     case content::DOWNLOAD_DANGER_TYPE_DANGEROUS_CONTENT:
       return "DANGEROUS_CONTENT";
     case content::DOWNLOAD_DANGER_TYPE_UNCOMMON_CONTENT:
       return "UNCOMMON_CONTENT";
     default:
-      // We shouldn't be returning a danger type string if it is
-      // NOT_DANGEROUS or MAYBE_DANGEROUS_CONTENT.
+      // Don't return a danger type string if it is NOT_DANGEROUS or
+      // MAYBE_DANGEROUS_CONTENT.
       NOTREACHED();
       return "";
   }
 }
 
 // Return a JSON dictionary containing some of the attributes of |download|.
 // The JSON dictionary will also have a field "id" set to |id|, and a field
 // "otr" set to |incognito|.
 DictionaryValue* CreateDownloadItemValue(
-    content::DownloadItem* download,
-    int id,
+    content::DownloadItem* download_item,
     bool incognito) {
   DictionaryValue* file_value = new DictionaryValue();
 
   file_value->SetInteger(
-      "started", static_cast<int>(download->GetStartTime().ToTimeT()));
+      "started", static_cast<int>(download_item->GetStartTime().ToTimeT()));
   file_value->SetString(
-      "since_string", TimeFormat::RelativeDate(download->GetStartTime(), NULL));
+      "since_string", TimeFormat::RelativeDate(
+          download_item->GetStartTime(), NULL));
   file_value->SetString(
-      "date_string", base::TimeFormatShortDate(download->GetStartTime()));
-  file_value->SetInteger("id", id);
+      "date_string", base::TimeFormatShortDate(download_item->GetStartTime()));
+  file_value->SetInteger("id", download_item->GetId());
 
-  FilePath download_path(download->GetTargetFilePath());
+  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());
 
   // Keep file names as LTR.
-  string16 file_name = download->GetFileNameToReportUser().LossyDisplayName();
+  string16 file_name =
+    download_item->GetFileNameToReportUser().LossyDisplayName();
   file_name = base::i18n::GetDisplayStringInLTRDirectionality(file_name);
   file_value->SetString("file_name", file_name);
-  file_value->SetString("url", download->GetURL().spec());
+  file_value->SetString("url", download_item->GetURL().spec());
   file_value->SetBoolean("otr", incognito);
-  file_value->SetInteger("total", static_cast<int>(download->GetTotalBytes()));
+  file_value->SetInteger("total", static_cast<int>(
+      download_item->GetTotalBytes()));
   file_value->SetBoolean("file_externally_removed",
-                         download->GetFileExternallyRemoved());
+                         download_item->GetFileExternallyRemoved());
 
-  if (download->IsInProgress()) {
-    if (download->GetSafetyState() == content::DownloadItem::DANGEROUS) {
+  if (download_item->IsInProgress()) {
+    if (download_item->GetSafetyState() == content::DownloadItem::DANGEROUS) {
       file_value->SetString("state", "DANGEROUS");
-      // These are the only danger states we expect to see (and the UI is
-      // equipped to handle):
-      DCHECK(download->GetDangerType() ==
+      // These are the only danger states that the UI is equipped to handle.
+      DCHECK(download_item->GetDangerType() ==
                  content::DOWNLOAD_DANGER_TYPE_DANGEROUS_FILE ||
-             download->GetDangerType() ==
+             download_item->GetDangerType() ==
                  content::DOWNLOAD_DANGER_TYPE_DANGEROUS_URL ||
-             download->GetDangerType() ==
+             download_item->GetDangerType() ==
                  content::DOWNLOAD_DANGER_TYPE_DANGEROUS_CONTENT ||
-             download->GetDangerType() ==
+             download_item->GetDangerType() ==
                  content::DOWNLOAD_DANGER_TYPE_UNCOMMON_CONTENT);
       const char* danger_type_value =
-          GetDangerTypeString(download->GetDangerType());
+          GetDangerTypeString(download_item->GetDangerType());
       file_value->SetString("danger_type", danger_type_value);
-    } else if (download->IsPaused()) {
+    } else if (download_item->IsPaused()) {
       file_value->SetString("state", "PAUSED");
     } else {
       file_value->SetString("state", "IN_PROGRESS");
     }
 
     file_value->SetString("progress_status_text",
-        download_util::GetProgressStatusText(download));
+        download_util::GetProgressStatusText(download_item));
 
     file_value->SetInteger("percent",
-        static_cast<int>(download->PercentComplete()));
+        static_cast<int>(download_item->PercentComplete()));
     file_value->SetInteger("received",
-        static_cast<int>(download->GetReceivedBytes()));
-  } else if (download->IsInterrupted()) {
+        static_cast<int>(download_item->GetReceivedBytes()));
+  } else if (download_item->IsInterrupted()) {
     file_value->SetString("state", "INTERRUPTED");
 
     file_value->SetString("progress_status_text",
-        download_util::GetProgressStatusText(download));
+        download_util::GetProgressStatusText(download_item));
 
     file_value->SetInteger("percent",
-        static_cast<int>(download->PercentComplete()));
+        static_cast<int>(download_item->PercentComplete()));
     file_value->SetInteger("received",
-        static_cast<int>(download->GetReceivedBytes()));
+        static_cast<int>(download_item->GetReceivedBytes()));
     file_value->SetString("last_reason_text",
         BaseDownloadItemModel::InterruptReasonMessage(
-            download->GetLastReason()));
-  } else if (download->IsCancelled()) {
+            download_item->GetLastReason()));
+  } else if (download_item->IsCancelled()) {
     file_value->SetString("state", "CANCELLED");
-  } else if (download->IsComplete()) {
-    if (download->GetSafetyState() == content::DownloadItem::DANGEROUS)
+  } else if (download_item->IsComplete()) {
+    if (download_item->GetSafetyState() == content::DownloadItem::DANGEROUS)
       file_value->SetString("state", "DANGEROUS");
     else
       file_value->SetString("state", "COMPLETE");
   } else {
     NOTREACHED() << "state undefined";
   }
 
   return file_value;
 }
 
 // Return true if |download_id| refers to a download that belongs to the
 // incognito download manager, if one exists.
 bool IsItemIncognito(
     int32 download_id,
     content::DownloadManager* manager,
     content::DownloadManager* original_manager) {
   // |original_manager| is only non-NULL if |manager| is incognito.
   return (original_manager &&
           (manager->GetDownload(download_id) != NULL));
 }
 
+// Filter out extension downloads and downloads that don't have a filename yet.

[James Hawkins, 2012/08/22 17:39:01]

nit: Filters

[benjhayden, 2012/08/22 20:19:22]

Done.

+bool IsDownloadDisplayable(const content::DownloadItem& item) {
+  return (!download_crx_util::IsExtensionDownload(item) &&
+          !item.IsTemporary() &&
+          !item.GetFileNameToReportUser().empty() &&
+          !item.GetTargetFilePath().empty());
+}
+
 }  // 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_)
     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)));
+                 weak_ptr_factory_.GetWeakPtr()));
 }
 
-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;
+void DownloadsDOMHandler::OnDownloadCreated(
+    content::DownloadManager* manager, content::DownloadItem* download_item) {
+  // DownloadsDOMHandler observes all items and only chooses which downloads to
+  // display in SendCurrentDownloads() and OnDownloadUpdated() using
+  // IsDownloadDisplayable().
+  download_item->AddObserver(this);
+  observing_items_.insert(download_item);
+  if (IsDownloadDisplayable(*download_item))
+    ScheduleSendCurrentDownloads();
+}
 
-  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);
+void DownloadsDOMHandler::OnDownloadUpdated(
+    content::DownloadItem* download_item) {
+  if (IsDownloadDisplayable(*download_item)) {
+    base::ListValue results_value;
+    results_value.Append(CreateDownloadItemValue(download_item, IsItemIncognito(
+        download_item->GetId(),
+        download_manager_,
+        original_profile_download_manager_)));
+    CallDownloadUpdated(results_value);
+  }
 }
 
 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_);
-  }
-
-  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++;
-  }
-
-  // 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);
-  }
-
-  SendCurrentDownloads();
+    content::DownloadItem* download_item) {
+  download_item->RemoveObserver(this);
+  observing_items_.erase(download_item);
+  ScheduleSendCurrentDownloads();
 }
 
 void DownloadsDOMHandler::ManagerGoingDown(content::DownloadManager* manager) {
   // This should never happen.  The lifetime of the DownloadsDOMHandler
   // is tied to the tab in which downloads.html is displayed, which cannot
   // outlive the Browser that contains it, which cannot outlive the Profile
   // it is associated with.  If that profile is an incognito profile,
   // it cannot outlive its original profile.  Thus this class should be
   // destroyed before a ManagerGoingDown() notification occurs.
   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)

[James Hawkins, 2012/08/22 17:39:01]

Under what circumstances is |web_contents| NULL?

[benjhayden, 2012/08/22 20:19:22]

Only the test.
GetWebUIWebContents(), CallDownloadsList(), and CallDownloadUpdated() exist
solely to facilitate tests.

[James Hawkins, 2012/08/22 20:24:20]

Please document this case (that |web_contents| is only NULL in tests).

[benjhayden, 2012/08/23 16:48:57]

Done.

+    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) {
     DCHECK(file->IsPersisted());
     file->Remove();
   }
 }
 
-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();
 
   // If this is an incognito downloader, clear All should clear main download
   // manager as well.
   if (original_profile_download_manager_)
     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());
 }
 
 // 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(
+      BrowserThread::UI, FROM_HERE,
+      base::Bind(&DownloadsDOMHandler::SendCurrentDownloads,
+                 weak_ptr_factory_.GetWeakPtr()));
+}
+
 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(),
-            download_manager_,
-            original_profile_download_manager_)));
+  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());
   // danger_prompt will delete itself.
   DCHECK(danger_prompt);
 }
 
 void DownloadsDOMHandler::DangerPromptAccepted(int download_id) {
   content::DownloadItem* item = download_manager_->GetActiveDownloadItem(
       download_id);
   if (!item)
     return;
   CountDownloadsDOMEvents(DOWNLOADS_DOM_EVENT_SAVE_DANGEROUS);
   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);
-  }
-  download_items_.clear();
+content::DownloadItem* DownloadsDOMHandler::GetDownloadByValue(
+    const base::ListValue* args) {
+  int id = -1;
+  if (!ExtractIntegerValue(args, &id))
+    return NULL;
+  content::DownloadItem* download_item = download_manager_->GetDownload(id);
+  if (download_item == NULL)
+    download_item = original_profile_download_manager_->GetDownload(id);
+  return download_item;
 }
 
-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);
-    }
-  }
-
-  return NULL;
+content::WebContents* DownloadsDOMHandler::GetWebUIWebContents() {
+  return web_ui()->GetWebContents();
 }
 
-content::DownloadItem* DownloadsDOMHandler::GetDownloadByValue(
-    const ListValue* args) {
-  int id;
-  if (ExtractIntegerValue(args, &id)) {
-    return GetDownloadById(id);
-  }
-  return NULL;
+void DownloadsDOMHandler::CallDownloadsList(const base::ListValue& downloads) {

[James Hawkins, 2012/08/22 17:39:01]

Remove these wrapper method and make the calls directly.

[benjhayden, 2012/08/22 20:19:22]

These wrapper methods facilitate the test.
I'm happy to discuss alternative ways to facilitate the test.
For example, a MockWebUI would be great, but I didn't see one in codesearch.

[James Hawkins, 2012/08/22 20:24:20]

Ah, right.  I forgot.

+  web_ui()->CallJavascriptFunction("downloadsList", downloads);
 }
+
+void DownloadsDOMHandler::CallDownloadUpdated(
+    const base::ListValue& download_item) {
+  web_ui()->CallJavascriptFunction("downloadUpdated", download_item);
+}