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

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/logging.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/supports_user_data.h"
 #include "base/threading/thread.h"
 #include "base/value_conversions.h"
@@ skipping 167 matching lines @@
         file_value->SetString("danger_type", danger_type_value);
       } else if (download_item->IsPaused()) {
         file_value->SetString("state", "PAUSED");
       } else {
         file_value->SetString("state", "IN_PROGRESS");
       }
       file_value->SetString("progress_status_text",
                             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;
 
     case content::DownloadItem::INTERRUPTED:
       file_value->SetString("state", "INTERRUPTED");
 
       file_value->SetString("progress_status_text",
                             download_model.GetTabProgressStatusText());
 
@@ skipping 102 matching lines @@
   web_ui()->RegisterMessageCallback("clearAll",
       base::Bind(&DownloadsDOMHandler::HandleClearAll,
                  weak_ptr_factory_.GetWeakPtr()));
   web_ui()->RegisterMessageCallback("openDownloadsFolder",
       base::Bind(&DownloadsDOMHandler::HandleOpenDownloadsFolder,
                  weak_ptr_factory_.GetWeakPtr()));
 }
 
 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(
     content::DownloadManager* manager,
     content::DownloadItem* download_item) {
   if (!DownloadItemModel(download_item).ShouldShowInShelf())
     return;
 
   // 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
   // at all downloads, and we do not tell it that |download_item| is being
   // removed. If DownloadManager is ever changed to not immediately remove
   // |download_item| from its map when OnDownloadRemoved is sent, then
   // DownloadsDOMHandler::OnDownloadRemoved() will need to explicitly tell
   // SendCurrentDownloads() that |download_item| was removed. A
   // SupportsUserData::Data would be the correct way to do this.
   ScheduleSendCurrentDownloads();
 }
 
 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();
 }
 
 void DownloadsDOMHandler::HandleOpenFile(const base::ListValue* args) {
   CountDownloadsDOMEvents(DOWNLOADS_DOM_EVENT_OPEN_FILE);
   content::DownloadItem* file = GetDownloadByValue(args);
   if (file)
     file->OpenDownload();
 }
 
@@ skipping 76 matching lines @@
   if (removals_.empty())
     return;
 
   const std::set<uint32> last_removed_ids = removals_.back();
   removals_.pop_back();
 
   for (auto id : last_removed_ids) {
     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);
   }
 }
 
 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) {
-  if (!IsDeletingHistoryAllowed())
+  if (!IsDeletingHistoryAllowed()) {
+    // This should only be reached during tests.
     return;
+  }
 
   CountDownloadsDOMEvents(DOWNLOADS_DOM_EVENT_CLEAR_ALL);
 
   std::vector<content::DownloadItem*> downloads;
   if (GetMainNotifierManager())
     GetMainNotifierManager()->GetAllDownloads(&downloads);
   if (original_notifier_ && original_notifier_->GetManager())
     original_notifier_->GetManager()->GetAllDownloads(&downloads);
   RemoveDownloads(downloads);
 }
 
 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() ||
         download->GetState() == content::DownloadItem::IN_PROGRESS) {
       continue;
     }
 
     item_model.SetShouldShowInShelf(false);
     ids.insert(download->GetId());
     download->UpdateObservers();
   }
+
   if (!ids.empty())
     removals_.push_back(ids);
 }
 
 void DownloadsDOMHandler::HandleOpenDownloadsFolder(
     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());
   }
 }
 
 // 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()));
 }
 
 content::DownloadManager* DownloadsDOMHandler::GetMainNotifierManager() {
   return main_notifier_.GetManager();
 }
 
 void DownloadsDOMHandler::FinalizeRemovals() {
   while (!removals_.empty()) {
     const std::set<uint32> remove = removals_.back();
     removals_.pop_back();
 
     for (const auto id : remove) {
       content::DownloadItem* download = GetDownloadById(id);
       if (download)
         download->Remove();
     }
   }
 }
 
 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(
     content::DownloadItem* dangerous_item) {
   DownloadDangerPrompt* danger_prompt = DownloadDangerPrompt::Create(
       dangerous_item,
       GetWebUIWebContents(),
       false,
       base::Bind(&DownloadsDOMHandler::DangerPromptDone,
                  weak_ptr_factory_.GetWeakPtr(), dangerous_item->GetId()));
@@ skipping 11 matching lines @@
   if (!item && original_notifier_.get() && original_notifier_->GetManager())
     item = original_notifier_->GetManager()->GetDownload(download_id);
   if (!item || item->IsDone())
     return;
   CountDownloadsDOMEvents(DOWNLOADS_DOM_EVENT_SAVE_DANGEROUS);
   item->ValidateDangerousDownload();
 }
 
 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(
     const base::ListValue* args) {
   std::string download_id;
   if (!args->GetString(0, &download_id)) {
     NOTREACHED();
     return nullptr;
   }
 
   uint64 id;
   if (!base::StringToUint64(download_id, &id)) {
     NOTREACHED();
     return nullptr;
   }
 
   return GetDownloadById(static_cast<uint32>(id));
 }
 
 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;
 }
 
 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);
 }