MD Downloads: track downloads in C++, dispatch discrete JS updates

chrome/browser/ui/webui/md_downloads/downloads_list_tracker.cc

Index: chrome/browser/ui/webui/md_downloads/downloads_list_tracker.cc
diff --git a/chrome/browser/ui/webui/md_downloads/downloads_list_tracker.cc b/chrome/browser/ui/webui/md_downloads/downloads_list_tracker.cc
new file mode 100644
index 0000000000000000000000000000000000000000..2dc2d6d45438ebab9781bf49d232dbc67e1e54ae
--- /dev/null
+++ b/chrome/browser/ui/webui/md_downloads/downloads_list_tracker.cc
@@ -0,0 +1,408 @@
+// Copyright 2015 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/md_downloads/downloads_list_tracker.h"
+
+#include "base/bind.h"
+#include "base/bind_helpers.h"
+#include "base/i18n/rtl.h"
+#include "base/strings/string16.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/time/time.h"
+#include "base/value_conversions.h"
+#include "chrome/browser/download/all_download_item_notifier.h"
+#include "chrome/browser/download/download_crx_util.h"
+#include "chrome/browser/download/download_item_model.h"
+#include "chrome/browser/download/download_query.h"
+#include "chrome/browser/extensions/api/downloads/downloads_api.h"
+#include "chrome/browser/extensions/extension_service.h"
+#include "chrome/browser/profiles/profile.h"
+#include "content/public/browser/browser_context.h"
+#include "content/public/browser/download_item.h"
+#include "content/public/browser/download_manager.h"
+#include "content/public/browser/web_ui.h"
+#include "extensions/browser/extension_system.h"
+#include "net/base/filename_util.h"
+#include "third_party/icu/source/i18n/unicode/datefmt.h"
+#include "ui/base/l10n/time_format.h"
+
+using content::BrowserContext;
+using content::DownloadItem;
+using content::DownloadManager;
+
+using DownloadVector = std::vector<DownloadItem*>;

[asanka, 2015/11/24 22:08:22]

Should this be:

  using DownloadVector = DownloadManager::DownloadVector

?

[Dan Beam, 2015/11/25 04:06:36]

Done.

+
+namespace {
+
+// Returns a string constant to be used as the |danger_type| value in
+// 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";
+    case content::DOWNLOAD_DANGER_TYPE_DANGEROUS_HOST:
+      return "DANGEROUS_HOST";
+    case content::DOWNLOAD_DANGER_TYPE_POTENTIALLY_UNWANTED:
+      return "POTENTIALLY_UNWANTED";
+    default:

[asanka, 2015/11/24 22:08:22]

I prefer avoiding the `default` label and instead adding case labels for the
NOT_DANGEROUS and MAYBE_DANGEROUS_CONTENT cases. Those cases can be handled the
same way you are handling the default case below.

This way if we introduce a new danger type, we'd be forced by the compiler to
update this mapping.

[Dan Beam, 2015/11/25 04:06:36]

Done.

+      // Don't return a danger type string if it is NOT_DANGEROUS or
+      // MAYBE_DANGEROUS_CONTENT.
+      NOTREACHED();
+      return "";
+  }
+}
+
+// TODO(dbeam): if useful elsewhere, move to base/i18n/time_formatting.h?
+base::string16 TimeFormatLongDate(const base::Time& time) {
+  scoped_ptr<icu::DateFormat> formatter(
+      icu::DateFormat::createDateInstance(icu::DateFormat::kLong));
+  icu::UnicodeString date_string;
+  formatter->format(static_cast<UDate>(time.ToDoubleT() * 1000), date_string);
+  return base::string16(date_string.getBuffer(),
+                        static_cast<size_t>(date_string.length()));
+}
+
+}  // namespace
+
+DownloadsListTracker::DownloadsListTracker(
+    DownloadManager* download_manager,
+    content::WebUI* web_ui)
+  : main_notifier_(download_manager, this),
+    web_ui_(web_ui),
+    should_show_(base::Bind(&DownloadsListTracker::ShouldShow,

[asanka, 2015/11/24 22:08:22]

Is this what git-cl format does?

[Dan Beam, 2015/11/25 04:06:36]

nope, it's what dan-fat-finger format does. Done.

+                 base::Unretained(this))) {
+  Init();
+}
+
+DownloadsListTracker::~DownloadsListTracker() {}
+
+void DownloadsListTracker::CallClearAll() {
+  if (sending_updates_)
+    web_ui_->CallJavascriptFunction("downloads.Manager.clearAll");
+}
+
+bool DownloadsListTracker::SetSearchTerms(const base::ListValue& search_terms) {
+  if (search_terms_.Equals(&search_terms))
+    return false;
+
+  search_terms_.Swap(search_terms.DeepCopy());
+  RebuildSortedSet();
+  return true;
+}
+
+void DownloadsListTracker::Start() {
+  sending_updates_ = true;
+  // TODO(dbeam): paging or limiting logic.
+  CallInsertItems(sorted_visible_items_.begin(), sorted_visible_items_.end());
+}
+
+void DownloadsListTracker::Stop() {
+  sending_updates_ = false;
+}
+
+DownloadManager* DownloadsListTracker::GetMainNotifierManager() const {
+  return main_notifier_.GetManager();
+}
+
+DownloadManager* DownloadsListTracker::GetOriginalNotifierManager() const {
+  return original_notifier_ ? original_notifier_->GetManager() : nullptr;
+}
+
+void DownloadsListTracker::OnDownloadCreated(DownloadManager* manager,
+                                             DownloadItem* download_item) {
+  if (should_show_.Run(*download_item))
+    CallInsertItem(sorted_visible_items_.insert(download_item).first);
+}
+
+void DownloadsListTracker::OnDownloadUpdated(DownloadManager* manager,
+                                             DownloadItem* download_item) {
+  auto current_position = sorted_visible_items_.find(download_item);
+  bool is_showing = current_position != sorted_visible_items_.end();
+  bool should_show = should_show_.Run(*download_item);
+
+  if (!is_showing && should_show)
+    CallInsertItem(sorted_visible_items_.insert(download_item).first);
+  else if (is_showing && !should_show)
+    RemoveItem(current_position);
+  else if (is_showing)
+    CallUpdateItem(current_position);
+}
+
+void DownloadsListTracker::OnDownloadRemoved(DownloadManager* manager,
+                                             DownloadItem* download_item) {
+  auto current_position = sorted_visible_items_.find(download_item);
+  if (current_position != sorted_visible_items_.end())
+    RemoveItem(current_position);
+}
+
+DownloadsListTracker::DownloadsListTracker(
+    DownloadManager* download_manager,
+    content::WebUI* web_ui,
+    base::Callback<bool(const DownloadItem&)> should_show)
+  : main_notifier_(download_manager, this),
+    web_ui_(web_ui),
+    should_show_(should_show) {
+  Init();
+}
+
+scoped_ptr<base::DictionaryValue> DownloadsListTracker::CreateDownloadItemValue(
+    content::DownloadItem* download_item) const {
+  // TODO(asanka): Move towards using download_model here for getting status and
+  // progress. The difference currently only matters to Drive downloads and
+  // those don't show up on the downloads page, but should.
+  DownloadItemModel download_model(download_item);
+
+  // The items which are to be written into file_value are also described in
+  // chrome/browser/resources/downloads/downloads.js in @typedef for
+  // BackendDownloadObject. Please update it whenever you add or remove
+  // any keys in file_value.
+  scoped_ptr<base::DictionaryValue> file_value(new base::DictionaryValue);
+
+  file_value->SetInteger(
+      "started", static_cast<int>(download_item->GetStartTime().ToTimeT()));
+  file_value->SetString(
+      "since_string", ui::TimeFormat::RelativeDate(
+          download_item->GetStartTime(), NULL));
+
+  base::Time start_time = download_item->GetStartTime();

[asanka, 2015/11/24 22:08:22]

If you're gonna store it in a variable, do so above the two other calls to
GetStartTime().

Although I'd be okay with continuing to call GetStartTime().

[Dan Beam, 2015/11/25 04:06:36]

Done.

+  base::string16 date_string = TimeFormatLongDate(start_time);
+  file_value->SetString("date_string", date_string);

[asanka, 2015/11/24 22:08:22]

Nit: Better naming for 'started', 'since_string', and 'date_string'?

[Dan Beam, 2015/11/25 04:06:36]

Let's do that separately.

+
+  file_value->SetString("id", base::Uint64ToString(download_item->GetId()));
+
+  base::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());
+
+  extensions::DownloadedByExtension* by_ext =
+      extensions::DownloadedByExtension::Get(download_item);
+  std::string by_ext_id;
+  std::string by_ext_name;
+  if (by_ext) {
+    by_ext_id = by_ext->id();
+    // TODO(dbeam): why doesn't DownloadsByExtension::name() return a string16?
+    by_ext_name = by_ext->name();
+
+    // Lookup the extension's current name() in case the user changed their
+    // language. This won't work if the extension was uninstalled, so the name
+    // might be the wrong language.
+    bool include_disabled = true;
+    const extensions::Extension* extension = extensions::ExtensionSystem::Get(
+        Profile::FromBrowserContext(download_item->GetBrowserContext()))->
+      extension_service()->GetExtensionById(by_ext->id(), include_disabled);
+    if (extension)
+      file_value->SetString("by_ext_name", extension->name());

[asanka, 2015/11/24 22:08:22]

This gets overwritten below.

[Dan Beam, 2015/11/25 04:06:36]

Done.

+  }
+  file_value->SetString("by_ext_id", by_ext_id);
+  file_value->SetString("by_ext_name", by_ext_name);
+
+  // Keep file names as LTR.
+  base::string16 file_name =
+    download_item->GetFileNameToReportUser().LossyDisplayName();
+  file_name = base::i18n::GetDisplayStringInLTRDirectionality(file_name);

[asanka, 2015/11/24 22:08:22]

Is this how we handle filenames? What if the filename needs to be displayed RTL?

[Dan Beam, 2015/11/25 04:06:36]

¯\_(ツ)_/¯

ask benjhayden@:
https://chromiumcodereview.appspot.com/10805020/diff/21013/chrome/browser/ui/...

fwiw: removing this line and running RTL (and pasting in RTL translations for
the file name) all seem to have similar results.

+  file_value->SetString("file_name", file_name);
+  file_value->SetString("url", download_item->GetURL().spec());
+  file_value->SetInteger("total", static_cast<int>(
+      download_item->GetTotalBytes()));
+  file_value->SetBoolean("file_externally_removed",
+                         download_item->GetFileExternallyRemoved());
+  file_value->SetBoolean("resume", download_item->CanResume());
+
+  bool incognito = false;
+  auto* original_manager = GetOriginalNotifierManager();
+  if (original_manager) {
+    incognito =
+        original_manager->GetDownload(download_item->GetId()) == download_item;
+  }
+  file_value->SetBoolean("otr", incognito);
+
+  const char* danger_type = "";
+  base::string16 last_reason_text;
+  // -2 is invalid, -1 means indeterminate, and 0-100 are in-progress.
+  int percent = -2;
+  base::string16 progress_status_text;
+  bool retry = false;
+  const char* state = nullptr;
+
+  switch (download_item->GetState()) {
+    case content::DownloadItem::IN_PROGRESS: {
+      if (download_item->IsDangerous()) {
+        state = "DANGEROUS";
+        danger_type = GetDangerTypeString(download_item->GetDangerType());
+      } else if (download_item->IsPaused()) {
+        state = "PAUSED";
+      } else {
+        state = "IN_PROGRESS";
+      }
+      progress_status_text = download_model.GetTabProgressStatusText();
+      percent = download_item->PercentComplete();
+      break;
+    }
+
+    case content::DownloadItem::INTERRUPTED:
+      state = "INTERRUPTED";
+      progress_status_text = download_model.GetTabProgressStatusText();
+
+      if (download_item->CanResume())
+        percent = download_item->PercentComplete();
+
+      last_reason_text = download_model.GetInterruptReasonText();
+      if (content::DOWNLOAD_INTERRUPT_REASON_CRASH ==
+          download_item->GetLastReason() && !download_item->CanResume()) {
+        retry = true;
+      }
+      break;
+
+    case content::DownloadItem::CANCELLED:
+      state = "CANCELLED";
+      retry = true;
+      break;
+
+    case content::DownloadItem::COMPLETE:
+      DCHECK(!download_item->IsDangerous());
+      state = "COMPLETE";
+      break;
+
+    case content::DownloadItem::MAX_DOWNLOAD_STATE:
+      NOTREACHED();
+  }
+
+  DCHECK(state);
+
+  file_value->SetString("danger_type", danger_type);
+  file_value->SetString("last_reason_text", last_reason_text);
+  file_value->SetInteger("percent", percent);
+  file_value->SetString("progress_status_text", progress_status_text);
+  file_value->SetBoolean("retry", retry);
+  file_value->SetString("state", state);
+
+  return file_value.Pass();
+}
+
+DownloadItem* DownloadsListTracker::GetItemForTesting(size_t index) {
+  if (index >= sorted_visible_items_.size())
+    return nullptr;
+
+  SortedSet::iterator it = sorted_visible_items_.begin();
+  std::advance(it, index);
+  return *it;
+}
+
+bool DownloadsListTracker::ShouldShow(const DownloadItem& item) const {
+  if (download_crx_util::IsExtensionDownload(item) ||
+      item.IsTemporary() ||
+      item.GetFileNameToReportUser().empty() ||
+      item.GetTargetFilePath().empty()) {
+    return false;
+  }
+
+  DownloadItem* mutable_item = const_cast<DownloadItem*>(&item);
+
+  DownloadItemModel model(mutable_item);
+  if (!model.ShouldShowInShelf())
+    return false;
+
+  if (search_terms_.empty())
+    return true;
+
+  DownloadVector all_items, filtered_items;
+  all_items.push_back(mutable_item);
+
+  DownloadQuery query;
+  query.AddFilter(DownloadQuery::FILTER_QUERY, search_terms_);
+  query.Search(all_items.begin(), all_items.end(), &filtered_items);
+
+  return !filtered_items.empty();
+}
+
+bool DownloadsListTracker::StartTimeComparator::operator() (
+    const content::DownloadItem* a, const content::DownloadItem* b) const {
+  return a->GetStartTime() > b->GetStartTime();
+}
+
+void DownloadsListTracker::Init() {
+  Profile* profile = Profile::FromBrowserContext(
+      GetMainNotifierManager()->GetBrowserContext());
+  if (profile->IsOffTheRecord()) {
+    original_notifier_.reset(new AllDownloadItemNotifier(
+       BrowserContext::GetDownloadManager(profile->GetOriginalProfile()),
+        this));
+  }
+
+  RebuildSortedSet();
+}
+
+void DownloadsListTracker::RebuildSortedSet() {
+  DownloadVector all_items, visible_items;
+
+  GetMainNotifierManager()->GetAllDownloads(&all_items);
+
+  if (GetOriginalNotifierManager())
+    GetOriginalNotifierManager()->GetAllDownloads(&all_items);
+
+  DownloadQuery query;
+  query.AddFilter(should_show_);

[asanka, 2015/11/24 22:08:22]

Shall we break up ShouldShow into two? This is a pretty expensive predicate
since each time it's invoked it runs DownloadQuery on a small array. It'll be
much more efficient to just apply the search terms to this query rather than
build and run a query for each download item.

[Dan Beam, 2015/11/25 04:06:36]

let me know if you find what I did acceptable.

[asanka, 2015/11/25 20:49:06]

Yeah. I think this is good.

+  query.Search(all_items.begin(), all_items.end(), &visible_items);
+
+  SortedSet sorted_visible_items(visible_items.begin(), visible_items.end());
+  sorted_visible_items_.swap(sorted_visible_items);
+}
+
+void DownloadsListTracker::CallInsertItem(const SortedSet::iterator& insert) {
+  if (!sending_updates_)
+    return;
+
+  base::ListValue list;
+  list.Append(CreateDownloadItemValue(*insert).Pass());
+
+  web_ui_->CallJavascriptFunction("downloads.Manager.insertItems",
+                                  base::FundamentalValue(GetIndex(insert)),
+                                  list);
+}
+
+void DownloadsListTracker::CallInsertItems(const SortedSet::iterator& start,
+                                           const SortedSet::iterator& end) {

[asanka, 2015/11/24 22:08:22]

This method only seems to be called once and only at the start where start,end
are begin()/end(). We don't actually want to 'insert', right? We want to set the
entire list of visible items to be the list [start,end).

I was initially a bit confused about why we need a method like this to insert a
batch of downloads, and then I realized we just want to insert all of
sorted_visible_items_. Can we simplify?

[Dan Beam, 2015/11/25 04:06:36]

yes, but the follow-up patch will insert chunks.  i'll simplify until then.

[asanka, 2015/11/25 20:49:06]

Acknowledged.

+  if (!sending_updates_)
+    return;
+
+  base::ListValue list;
+  for (auto it = start; it != end; ++it) {
+    list.Append(CreateDownloadItemValue(*it).Pass());
+  }
+
+  web_ui_->CallJavascriptFunction("downloads.Manager.insertItems",
+                                  base::FundamentalValue(GetIndex(start)),
+                                  list);
+}
+
+void DownloadsListTracker::CallUpdateItem(const SortedSet::iterator& update) {
+  if (!sending_updates_)
+    return;
+
+  web_ui_->CallJavascriptFunction("downloads.Manager.updateItem",
+                                  base::FundamentalValue(GetIndex(update)),
+                                  *CreateDownloadItemValue(*update));
+}
+
+int DownloadsListTracker::GetIndex(const SortedSet::iterator& position) const {
+  // TODO(dbeam): this could be log(N) if |position| was random access.
+  return std::distance(sorted_visible_items_.begin(), position);
+}
+
+void DownloadsListTracker::RemoveItem(const SortedSet::iterator& remove) {
+  if (sending_updates_) {
+    web_ui_->CallJavascriptFunction("downloads.Manager.removeItem",
+                                    base::FundamentalValue(GetIndex(remove)));
+  }
+  sorted_visible_items_.erase(remove);
+}