MD Downloads: limit the amount of downloads we send

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

 // 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 <iterator>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
@@ skipping 20 matching lines @@
 #include "ui/base/l10n/time_format.h"
 
 using content::BrowserContext;
 using content::DownloadItem;
 using content::DownloadManager;
 
 using DownloadVector = DownloadManager::DownloadVector;
 
 namespace {
 
+const size_t kChunkSize = 20u;
+
 // 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";
@@ skipping 34 matching lines @@
     content::WebUI* web_ui)
   : main_notifier_(download_manager, this),
     web_ui_(web_ui),
     should_show_(base::Bind(&DownloadsListTracker::ShouldShow,
                             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) {
   std::vector<base::string16> new_terms;
   new_terms.resize(search_terms.GetSize());
 
   for (size_t i = 0; i < search_terms.GetSize(); ++i)
     search_terms.GetString(i, &new_terms[i]);
 
   if (new_terms == search_terms_)
     return false;
 
   search_terms_.swap(new_terms);
-  RebuildSortedSet();
+  RebuildSortedItems();
   return true;
 }
 
-void DownloadsListTracker::Start() {
+void DownloadsListTracker::StartAndSendChunk() {
   sending_updates_ = true;
 
-  // TODO(dbeam): paging and limiting logic.
+  CHECK_LE(sent_to_page_, sorted_items_.size());
+
+  SortedSet::iterator it = sorted_items_.begin();
+  std::advance(it, sent_to_page_);
 
   base::ListValue list;
-  for (auto* item : sorted_visible_items_)
-    list.Append(CreateDownloadItemValue(item).Pass());
+  while (it != sorted_items_.end() && list.GetSize() < kChunkSize) {
+    list.Append(CreateDownloadItemValue(*it).Pass());
+    ++it;
+  }
 
-  web_ui_->CallJavascriptFunction("downloads.Manager.insertItems",
-                                  base::FundamentalValue(0), list);
+  web_ui_->CallJavascriptFunction(
+      "downloads.Manager.insertItems",
+      base::FundamentalValue(static_cast<int>(sent_to_page_)),
+      list);
+
+  sent_to_page_ += list.GetSize();
 }
 
-void DownloadsListTracker::Stop() {
+void DownloadsListTracker::Reset() {
+  if (sending_updates_)
+    web_ui_->CallJavascriptFunction("downloads.Manager.clearAll");
+
   sending_updates_ = false;
+  sent_to_page_ = 0u;
 }
 
 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) {
+  DCHECK_EQ(0u, sorted_items_.count(download_item));
   if (should_show_.Run(*download_item))
-    CallInsertItem(sorted_visible_items_.insert(download_item).first);
+    InsertItem(sorted_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();
+  auto current_position = sorted_items_.find(download_item);
+  bool is_showing = current_position != sorted_items_.end();
   bool should_show = should_show_.Run(*download_item);
 
   if (!is_showing && should_show)
-    CallInsertItem(sorted_visible_items_.insert(download_item).first);
+    InsertItem(sorted_items_.insert(download_item).first);
   else if (is_showing && !should_show)
     RemoveItem(current_position);
   else if (is_showing)
-    CallUpdateItem(current_position);
+    UpdateItem(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())
+  auto current_position = sorted_items_.find(download_item);
+  if (current_position != sorted_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) {
@@ skipping 128 matching lines @@
   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();
 }
 
 const DownloadItem* DownloadsListTracker::GetItemForTesting(size_t index)
     const {
-  if (index >= sorted_visible_items_.size())
+  if (index >= sorted_items_.size())
     return nullptr;
 
-  SortedSet::iterator it = sorted_visible_items_.begin();
+  SortedSet::iterator it = sorted_items_.begin();
   std::advance(it, index);
   return *it;
 }
 
 bool DownloadsListTracker::ShouldShow(const DownloadItem& item) const {
   return !download_crx_util::IsExtensionDownload(item) &&
       !item.IsTemporary() &&
       !item.GetFileNameToReportUser().empty() &&
       !item.GetTargetFilePath().empty() &&
       DownloadItemModel(const_cast<DownloadItem*>(&item)).ShouldShowInShelf() &&
       DownloadQuery::MatchesQuery(search_terms_, item);
 }
 
 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();
+  RebuildSortedItems();
 }
 
-void DownloadsListTracker::RebuildSortedSet() {
+void DownloadsListTracker::RebuildSortedItems() {
   DownloadVector all_items, visible_items;
 
   GetMainNotifierManager()->GetAllDownloads(&all_items);
 
   if (GetOriginalNotifierManager())
     GetOriginalNotifierManager()->GetAllDownloads(&all_items);
 
   DownloadQuery query;
   query.AddFilter(should_show_);
   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);
+  SortedSet sorted_items(visible_items.begin(), visible_items.end());
+  sorted_items_.swap(sorted_items);
 }
 
-void DownloadsListTracker::CallInsertItem(const SortedSet::iterator& insert) {
+void DownloadsListTracker::InsertItem(const SortedSet::iterator& insert) {
   if (!sending_updates_)
     return;
 
+  size_t index = GetIndex(insert);
+  if (index > kChunkSize && index >= sent_to_page_)

[asanka, 2015/12/04 21:03:56]

What's the point of the (index > kChunkSize)? Don't you just want to avoid
exceeding the current kChunkSize block? I.e. index >= ((sent_to_page_ - 1) /
kChunkSize + 1) * kChunkSize) ... which requires that sent_to_page_ is signed.

[Dan Beam, 2015/12/04 22:52:35]

when the first download ever is created, sent_to_page_ == 0 and inserts are
ignored.  we really only want to ignore downloads offscreen, so I chose to
always insert up to kChunkSize

[Dan Beam, 2015/12/05 00:31:43]

and if you were wondering how we avoid pathological cases like undoing a "clear
all" inserting ALL the downloads, one by one, and updating sent_to_page_ each
time...

[asanka, 2015/12/07 20:31:50]

Ok. My misunderstanding was that I thought this code wanted to send the new
download if it would've sent the download in response to the last getDownloads
call. I'm okay with either behavior, but it's user visible so you should weigh
user expectations into this.

Should there be some sort of indication that there are more downloads on the
page? Okay if not, I'm just trying to understand the design.

[Dan Beam, 2015/12/09 06:55:21]

the only way users currently know if there are more downloads is by scrolling
down further.  fortunately, I think this is a pretty common UI paradigm (see:
facebook timeline).

i'm fine with trying to think of something useful to show, but I'd rather let
designers do it (and not block this CL).

+    return;
+
   base::ListValue list;
   list.Append(CreateDownloadItemValue(*insert).Pass());
 
-  web_ui_->CallJavascriptFunction("downloads.Manager.insertItems",
-                                  base::FundamentalValue(GetIndex(insert)),
-                                  list);
+  web_ui_->CallJavascriptFunction(
+      "downloads.Manager.insertItems",
+      base::FundamentalValue(static_cast<int>(index)),
+      list);
+
+  sent_to_page_++;
 }
 
-void DownloadsListTracker::CallUpdateItem(const SortedSet::iterator& update) {
-  if (!sending_updates_)
+void DownloadsListTracker::UpdateItem(const SortedSet::iterator& update) {
+  if (!sending_updates_ || GetIndex(update) >= sent_to_page_)
     return;
 
-  web_ui_->CallJavascriptFunction("downloads.Manager.updateItem",
-                                  base::FundamentalValue(GetIndex(update)),
-                                  *CreateDownloadItemValue(*update));
+  web_ui_->CallJavascriptFunction(
+      "downloads.Manager.updateItem",
+      base::FundamentalValue(static_cast<int>(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);
+size_t DownloadsListTracker::GetIndex(const SortedSet::iterator& item) const {
+  // TODO(dbeam): this could be log(N) if |item| was random access.
+  return std::distance(sorted_items_.begin(), item);
 }
 
 void DownloadsListTracker::RemoveItem(const SortedSet::iterator& remove) {
   if (sending_updates_) {
-    web_ui_->CallJavascriptFunction("downloads.Manager.removeItem",
-                                    base::FundamentalValue(GetIndex(remove)));
+    size_t index = GetIndex(remove);
+    if (index < sent_to_page_) {
+      web_ui_->CallJavascriptFunction(
+          "downloads.Manager.removeItem",
+          base::FundamentalValue(static_cast<int>(index)));
+      sent_to_page_--;
+    }
   }
-  sorted_visible_items_.erase(remove);
+  sorted_items_.erase(remove);
 }