downloads: break downloads.js into more classes/files.

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 1a1dbc90aafa9cb331b7f04107dcc4364e0b0920..48784f3027559c3284e29b8e69bc8094cec1ada5 100644
--- a/chrome/browser/ui/webui/downloads_dom_handler.cc
+++ b/chrome/browser/ui/webui/downloads_dom_handler.cc
@@ -12,6 +12,7 @@
 #include "base/bind_helpers.h"
 #include "base/i18n/rtl.h"
 #include "base/i18n/time_formatting.h"
+#include "base/logging.h"
 #include "base/memory/singleton.h"
 #include "base/metrics/field_trial.h"
 #include "base/metrics/histogram.h"
@@ -199,7 +200,7 @@ base::DictionaryValue* CreateDownloadItemValue(
                             download_model.GetTabProgressStatusText());
 
       file_value->SetInteger("percent",
-          static_cast<int>(download_item->PercentComplete()));
+          std::max(0, static_cast<int>(download_item->PercentComplete())));
       file_value->SetInteger("received",
           static_cast<int>(download_item->GetReceivedBytes()));
       break;
@@ -322,35 +323,49 @@ void DownloadsDOMHandler::RegisterMessages() {
 
 void DownloadsDOMHandler::OnDownloadCreated(
     content::DownloadManager* manager, content::DownloadItem* download_item) {
-  if (IsDownloadDisplayable(*download_item))
-    ScheduleSendCurrentDownloads();
+  if (!IsDownloadDisplayable(*download_item))
+    new_downloads_.insert(download_item->GetId());

[asanka, 2015/03/10 04:15:51]

If OnDownloadCreated() is called for a download restored from history (e.g.
because the history DB is slow and chrome://downloads was opened before history
could be read), then it's possible there won't be another OnDownloadUpdated() to
cause the downloads to be sent.

[Dan Beam, 2015/03/10 05:41:02]

added back the original code

 }
 
 void DownloadsDOMHandler::OnDownloadUpdated(
     content::DownloadManager* manager,
     content::DownloadItem* download_item) {
-  if (IsDownloadDisplayable(*download_item)) {
-    if (search_terms_ && !search_terms_->empty()) {
-      // Don't CallDownloadUpdated() if download_item doesn't match
-      // search_terms_.
-      // TODO(benjhayden): Consider splitting MatchesQuery() out to a function.
-      content::DownloadManager::DownloadVector all_items, filtered_items;
-      all_items.push_back(download_item);
-      DownloadQuery query;
-      query.AddFilter(DownloadQuery::FILTER_QUERY, *search_terms_.get());
-      query.Search(all_items.begin(), all_items.end(), &filtered_items);
-      if (filtered_items.empty())
-        return;
+  const uint32 download_id = download_item->GetId();
+  if (new_downloads_.count(download_id)) {
+    // New downloads must be sent before they can be updated.
+    if (!IsDownloadDisplayable(*download_item)) {
+      // Item isn't ready to be displayed yet. Wait until it is.
+      return;
     }
-    base::ListValue results_value;
-    results_value.Append(CreateDownloadItemValue(
-        download_item,
-        (original_notifier_.get() &&
-          (manager == main_notifier_.GetManager()))));
-    CallDownloadUpdated(results_value);
-  } else {
+    // Send rather than schedule as this must happen before |CallUpdateItem|.
+    SendCurrentDownloads();

[asanka, 2015/03/10 04:15:51]

If we go this route, then a subsequent CallUpdateItem() would be redundant.

[Dan Beam, 2015/03/10 05:41:03]

Done.

+    new_downloads_.erase(download_id);
+  }
+
+  if (DownloadItemModel(download_item).IsBeingRevived() ||
+      !IsDownloadDisplayable(*download_item)) {

[asanka, 2015/03/10 04:15:51]

IsDownloadDisplayable() could be returning false for a download that's never
supposed to be shown in our UI (e.g. it's an app download). If new_downloads_
were to be emptied, then ScheduleSendCurrentDownloads() would be called on each
update of such a download.

[Dan Beam, 2015/03/10 05:41:03]

this matches what the current code does

[asanka, 2015/03/11 19:57:49]

True, but it's still going to be quite expensive to send all the downloads on
every update. We'd also run into this case if chrome://downloads is opened while
an app download is in progress.

If it's not going to be addressed here, we should make a note of it to address
this later.

+    // A download will be shown or hidden by this update. Resend the list.
     ScheduleSendCurrentDownloads();
+    return;
   }
+
+  if (search_terms_ && !search_terms_->empty()) {
+    // Don't CallUpdateItem() if download_item doesn't match
+    // search_terms_.
+    // TODO(benjhayden): Consider splitting MatchesQuery() out to a function.
+    content::DownloadManager::DownloadVector all_items, filtered_items;
+    all_items.push_back(download_item);
+    DownloadQuery query;
+    query.AddFilter(DownloadQuery::FILTER_QUERY, *search_terms_);
+    query.Search(all_items.begin(), all_items.end(), &filtered_items);
+    if (filtered_items.empty())
+      return;
+  }
+
+  scoped_ptr<base::DictionaryValue> item(CreateDownloadItemValue(
+      download_item,
+      original_notifier_ && manager == main_notifier_.GetManager()));
+  CallUpdateItem(*item);
 }
 
 void DownloadsDOMHandler::OnDownloadRemoved(
@@ -374,7 +389,7 @@ void DownloadsDOMHandler::OnDownloadRemoved(
 
 void DownloadsDOMHandler::HandleGetDownloads(const base::ListValue* args) {
   CountDownloadsDOMEvents(DOWNLOADS_DOM_EVENT_GET_DOWNLOADS);
-  search_terms_.reset((args && !args->empty()) ? args->DeepCopy() : NULL);
+  search_terms_.reset(args && !args->empty() ? args->DeepCopy() : NULL);
   SendCurrentDownloads();
 }
 
@@ -471,8 +486,14 @@ void DownloadsDOMHandler::HandleUndo(const base::ListValue* args) {
     content::DownloadItem* download = GetDownloadById(id);
     if (!download)
       continue;
-    DownloadItemModel(download).SetShouldShowInShelf(true);
+
+    DownloadItemModel model(download);
+    model.SetShouldShowInShelf(true);

[asanka, 2015/03/10 04:15:51]

Why was this necessary?

[Dan Beam, 2015/03/10 05:41:02]

DownloadsDOMHandler::Handle{Remove,ClearAll}() run
DownloadItemModel(item).SetShouldShowInShelf(false) on each item to ensure
IsDownloadDisplayable(*item) returns false.

when the removal is undone, this needs to be reversed so the item is visible
again.

+    model.SetIsBeingRevived(true);

[asanka, 2015/03/10 04:15:51]

It seems the only purpose of SetIsBeingRevived() is to suppress CallUpdateItem()
and instead call ScheduleSendCurrentDownloads(). That could be accomplished by
pre-emptively calling ScheduleSendCurrentDownloads() when undoing a deletion and
checking for update_scheduled_ in OnDownloadUpdated(). CallUpdateItem() is
redudant if there's a scheduled update, so that'll be something we should
consider doing anyway.

[Dan Beam, 2015/03/10 05:41:02]

that would work with only 1 handler.

ALL handlers need to know about this state, which is why we decided to put it in
the model.
that's fixed now

+
     download->UpdateObservers();
+
+    model.SetIsBeingRevived(false);
   }
 }
 
@@ -484,8 +505,10 @@ void DownloadsDOMHandler::HandleCancel(const base::ListValue* args) {
 }
 
 void DownloadsDOMHandler::HandleClearAll(const base::ListValue* args) {
-  if (!IsDeletingHistoryAllowed())
+  if (!IsDeletingHistoryAllowed()) {
+    // This should only be reached during tests.
     return;
+  }
 
   CountDownloadsDOMEvents(DOWNLOADS_DOM_EVENT_CLEAR_ALL);
 
@@ -500,6 +523,7 @@ void DownloadsDOMHandler::HandleClearAll(const base::ListValue* args) {
 void DownloadsDOMHandler::RemoveDownloads(
     const std::vector<content::DownloadItem*>& to_remove) {
   std::set<uint32> ids;
+
   for (auto* download : to_remove) {
     DownloadItemModel item_model(download);
     if (!item_model.ShouldShowInShelf() ||
@@ -511,6 +535,7 @@ void DownloadsDOMHandler::RemoveDownloads(
     ids.insert(download->GetId());
     download->UpdateObservers();
   }
+
   if (!ids.empty())
     removals_.push_back(ids);
 }
@@ -534,7 +559,9 @@ void DownloadsDOMHandler::ScheduleSendCurrentDownloads() {
   // 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,
@@ -560,34 +587,33 @@ void DownloadsDOMHandler::FinalizeRemovals() {
 
 void DownloadsDOMHandler::SendCurrentDownloads() {
   update_scheduled_ = false;
+
   content::DownloadManager::DownloadVector all_items, filtered_items;
   if (main_notifier_.GetManager()) {
     main_notifier_.GetManager()->GetAllDownloads(&all_items);
     main_notifier_.GetManager()->CheckForHistoryFilesRemoval();
   }
-  if (original_notifier_.get() && original_notifier_->GetManager()) {
+  if (original_notifier_ && original_notifier_->GetManager()) {
     original_notifier_->GetManager()->GetAllDownloads(&all_items);
     original_notifier_->GetManager()->CheckForHistoryFilesRemoval();
   }
+
   DownloadQuery query;
-  if (search_terms_ && !search_terms_->empty()) {
-    query.AddFilter(DownloadQuery::FILTER_QUERY, *search_terms_.get());
-  }
+  if (search_terms_ && !search_terms_->empty())
+    query.AddFilter(DownloadQuery::FILTER_QUERY, *search_terms_);
   query.AddFilter(base::Bind(&IsDownloadDisplayable));
   query.AddSorter(DownloadQuery::SORT_START_TIME, DownloadQuery::DESCENDING);
   query.Limit(kMaxDownloads);
   query.Search(all_items.begin(), all_items.end(), &filtered_items);
+
   base::ListValue results_value;
-  for (content::DownloadManager::DownloadVector::iterator
-       iter = filtered_items.begin(); iter != filtered_items.end(); ++iter) {
+  for (auto it = filtered_items.begin(); it != filtered_items.end(); ++it) {

[asanka, 2015/03/10 04:15:51]

Use a range based for instead.

[Dan Beam, 2015/03/10 05:41:02]

Done.

     results_value.Append(CreateDownloadItemValue(
-        *iter,
-        (original_notifier_.get() &&
-          main_notifier_.GetManager() &&
-          (main_notifier_.GetManager()->GetDownload((*iter)->GetId()) ==
-          *iter))));
+        *it,
+        original_notifier_ && main_notifier_.GetManager() &&
+            main_notifier_.GetManager()->GetDownload((*it)->GetId()) == *it));
   }
-  CallDownloadsList(results_value);
+  CallUpdateAll(results_value);
 }
 
 void DownloadsDOMHandler::ShowDangerPrompt(
@@ -619,9 +645,9 @@ void DownloadsDOMHandler::DangerPromptDone(
 
 bool DownloadsDOMHandler::IsDeletingHistoryAllowed() {
   content::DownloadManager* manager = main_notifier_.GetManager();
-  return (manager &&
-          Profile::FromBrowserContext(manager->GetBrowserContext())->
-              GetPrefs()->GetBoolean(prefs::kAllowDeletingBrowserHistory));
+  return manager &&
+         Profile::FromBrowserContext(manager->GetBrowserContext())->
+             GetPrefs()->GetBoolean(prefs::kAllowDeletingBrowserHistory);
 }
 
 content::DownloadItem* DownloadsDOMHandler::GetDownloadByValue(
@@ -645,7 +671,7 @@ content::DownloadItem* DownloadsDOMHandler::GetDownloadById(uint32 id) {
   content::DownloadItem* item = NULL;
   if (GetMainNotifierManager())
     item = GetMainNotifierManager()->GetDownload(id);
-  if (!item && original_notifier_.get() && original_notifier_->GetManager())
+  if (!item && original_notifier_ && original_notifier_->GetManager())
     item = original_notifier_->GetManager()->GetDownload(id);
   return item;
 }
@@ -654,11 +680,10 @@ content::WebContents* DownloadsDOMHandler::GetWebUIWebContents() {
   return web_ui()->GetWebContents();
 }
 
-void DownloadsDOMHandler::CallDownloadsList(const base::ListValue& downloads) {
-  web_ui()->CallJavascriptFunction("downloadsList", downloads);
+void DownloadsDOMHandler::CallUpdateAll(const base::ListValue& list) {
+  web_ui()->CallJavascriptFunction("downloads.Manager.updateAll", list);
 }
 
-void DownloadsDOMHandler::CallDownloadUpdated(
-    const base::ListValue& download_item) {
-  web_ui()->CallJavascriptFunction("downloadUpdated", download_item);
+void DownloadsDOMHandler::CallUpdateItem(const base::DictionaryValue& item) {
+  web_ui()->CallJavascriptFunction("downloads.Manager.updateItem", item);
 }