MD Downloads: limit the amount of downloads we send

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
index b163644474913c5ee97c468b61515d6772843280..9ce99018ae4384b7ac4b7c9ee9641f0f69b34073 100644
--- a/chrome/browser/ui/webui/md_downloads/downloads_list_tracker.cc
+++ b/chrome/browser/ui/webui/md_downloads/downloads_list_tracker.cc
@@ -38,6 +38,8 @@ 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
@@ -92,11 +94,6 @@ DownloadsListTracker::DownloadsListTracker(
 
 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());
@@ -108,25 +105,38 @@ bool DownloadsListTracker::SetSearchTerms(const base::ListValue& 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(static_cast<int>(sent_to_page_)),
+      list);
 
-  web_ui_->CallJavascriptFunction("downloads.Manager.insertItems",
-                                  base::FundamentalValue(0), 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 {
@@ -139,28 +149,29 @@ DownloadManager* DownloadsListTracker::GetOriginalNotifierManager() const {
 
 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);
 }
 
@@ -309,10 +320,10 @@ scoped_ptr<base::DictionaryValue> DownloadsListTracker::CreateDownloadItemValue(
 
 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;
 }
@@ -340,10 +351,10 @@ void DownloadsListTracker::Init() {
         this));
   }
 
-  RebuildSortedSet();
+  RebuildSortedItems();
 }
 
-void DownloadsListTracker::RebuildSortedSet() {
+void DownloadsListTracker::RebuildSortedItems() {
   DownloadVector all_items, visible_items;
 
   GetMainNotifierManager()->GetAllDownloads(&all_items);
@@ -355,40 +366,53 @@ void DownloadsListTracker::RebuildSortedSet() {
   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);
 }