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..3230944700a036cc20d25d44b753b0f607660ee7 100644
--- a/chrome/browser/ui/webui/downloads_dom_handler.cc
+++ b/chrome/browser/ui/webui/downloads_dom_handler.cc
@@ -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));
@@ -220,154 +219,123 @@ 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);
   }
 }
 
 DownloadsDOMHandler::~DownloadsDOMHandler() {
-  ClearDownloadItems();
+  // Make SearchDownloads() return all the items.
+  search_text_ = std::wstring();
+  content::DownloadManager::DownloadVector downloads;
+  SearchDownloads(&downloads);
+  for (content::DownloadManager::DownloadVector::const_iterator
+           iter = downloads.begin();
+       iter != downloads.end(); ++iter) {
+    (*iter)->RemoveObserver(this);
+  }

[Randy Smith (Not in Mondays), 2012/08/15 19:04:16]

This is quite scary to me--you're relying on the search (which is a reasonably
high level function) always returning the same downloads, even if there's a
change in their state.  Yes, it *should* work, but if it doesn't, you've got a
use-after-free error.  The usual pattern is to track all the items observed, and
remove those items when you remove observation, and I'd rather keep that
pattern.

[benjhayden, 2012/08/16 18:52:09]

Done, but I made a note to write AddObserver(WeakPtr<Observer>).

   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)));
+                 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;
-
-  const int id = static_cast<int>(it - download_items_.begin());
+void DownloadsDOMHandler::OnDownloadCreated(
+    content::DownloadManager* manager, content::DownloadItem* download_item) {
+  download_item->AddObserver(this);
+  if (!update_scheduled_) {

[Randy Smith (Not in Mondays), 2012/08/15 19:04:16]

Comment as to motivation.

[benjhayden, 2012/08/16 18:52:09]

Done.

+    update_scheduled_ = true;
+    BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
+        base::Bind(&DownloadsDOMHandler::SendCurrentDownloads,
+                   weak_ptr_factory_.GetWeakPtr()));
+  }
+}
 
-  ListValue results_value;
-  results_value.Append(CreateDownloadItemValue(download, id, IsItemIncognito(
+void DownloadsDOMHandler::OnDownloadUpdated(content::DownloadItem* download) {
+  base::ListValue results_value;
+  results_value.Append(CreateDownloadItemValue(download, IsItemIncognito(
       download->GetId(),
       download_manager_,
       original_profile_download_manager_)));
-  web_ui()->CallJavascriptFunction("downloadUpdated", results_value);
+  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++;
+  if (!update_scheduled_) {
+    update_scheduled_ = true;
+    BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
+        base::Bind(&DownloadsDOMHandler::SendCurrentDownloads,
+                   weak_ptr_factory_.GetWeakPtr()));
   }
-
-  // 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();
 }
 
 void DownloadsDOMHandler::ManagerGoingDown(content::DownloadManager* manager) {
@@ -380,73 +348,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_ = UTF16ToWideHack(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);
-    }
+  if (!file)
+    return;
+  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);
   }
 }
 
-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.

[Randy Smith (Not in Mondays), 2012/08/15 19:04:16]

How does this end up working?  Do we tear down the danger prompt if the download
disappears out from under us?

[benjhayden, 2012/08/16 18:52:09]

The danger prompt owns itself, so we hand it a weak ptr in its callbacks. If the
tab is closed before the danger prompt is accepted/canceled, then the weak ptr
will say "nevermind", and the WebContents will make sure that the danger prompt
cleans itself up.
As for downloads disappearing from under us, we notice as soon as possible
(OnDownloadDestroyed), and update the renderer in the next UI message loop
iteration.
GetDownloadByValue now uses DownloadManager::GetDownload(), so if the download
still exists, then we'll get it, and if it doesn't, then we won't.

 }
 
-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 +415,15 @@ 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) {
+  // TODO(benjhayden): Should this only clear items that match search_text_?

[Randy Smith (Not in Mondays), 2012/08/15 19:04:16]

I vote yes, but obviously not in this CL.

[benjhayden, 2012/08/16 18:52:09]

Done.

   CountDownloadsDOMEvents(DOWNLOADS_DOM_EVENT_CLEAR_ALL);
   download_manager_->RemoveAllDownloads();
 
@@ -472,7 +433,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 +443,42 @@ 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) {
+    // Filter out extension downloads.
+    if (!download_crx_util::IsExtensionDownload(**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) {
+  // TODO(benjhayden): Use DownloadQuery, simplify DownloadManager interface
+  download_manager_->SearchDownloads(WideToUTF16(search_text_), downloads);
+  if (original_profile_download_manager_) {
+    original_profile_download_manager_->SearchDownloads(
+        WideToUTF16(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 +495,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);
 }