Cancel the JumpList update if top 5 most visited URLs are unchanged

chrome/browser/win/jumplist.cc

Index: chrome/browser/win/jumplist.cc
diff --git a/chrome/browser/win/jumplist.cc b/chrome/browser/win/jumplist.cc
index 7f8ecf1257bda62023d10ea3bf2b181b72fe7a6c..16bd831c2fd2364417b67bd1b24f8cf1376c8dfc 100644
--- a/chrome/browser/win/jumplist.cc
+++ b/chrome/browser/win/jumplist.cc
@@ -292,6 +292,31 @@ void JumpList::OnMostVisitedURLsAvailable(
   {
     JumpListData* data = &jumplist_data_->data;
     base::AutoLock auto_lock(data->list_lock_);
+
+    bool need_update = false;

[grt (UTC plus 2), 2017/05/24 07:22:12]

i think it makes sense to make a helper function for this new code. this here
could then be:

  // There is nothing to do if the top most visited sites have not changed.
  if (!HasNewMostVisitedItems(data->most_visited_pages_, urls))
    return;

then you could also write a unit test for this function that would catch the
out-of-bounds read below.

[chengx, 2017/05/24 23:44:14]

I've added the helper function. I think it's better to put this method in the
anonymous namespace, so it's very tricky to make a unit test for it. There's
also a bug filed for adding a unit test suite for JumpList class. Honestly, I
have put it aside now as it's more complicated.

+
+    // If the number of the top sites going to be displayed is larger than the

[grt (UTC plus 2), 2017/05/24 07:22:12]

although it's unexpected, i think you also want to schedule an update if
urls.size() < data->most_visited_pages_.size(), no? otherwise the loop below
will run off the end of |urls|.

[chengx, 2017/05/24 23:44:14]

Done. You're right.

+    // current one, schedule an update.
+    size_t topsites_count_updated = std::min(urls.size(), kMostVisitedItems);
+    if (topsites_count_updated > data->most_visited_pages_.size())
+      need_update = true;
+
+    // Cancel this jumplist update if the top |kMostVisitedItems| most visited
+    // urls are unchanged.
+    if (!need_update) {

[grt (UTC plus 2), 2017/05/24 07:22:12]

how about something like this:

#include <algorithm>
#include <iterator>

    if (std::equal(std::begin(data->most_visited_pages_),
                   std::end(data->most_visited_pages_),
                   std::begin(urls),
                   [](const auto& item_ptr, const auto& most_visited_url) {
                     return item_ptr->url() == most_visited_url.url.spec();
                   })) {
      return;
    }

[chengx, 2017/05/24 23:44:14]

Done. This is really impressive!

+      size_t i = 0;
+      for (ShellLinkItemList::const_iterator
+               iter = data->most_visited_pages_.begin();
+           iter != data->most_visited_pages_.end(); ++iter, ++i) {
+        if ((*iter)->url() != urls[i].url.spec()) {
+          need_update = true;
+          break;
+        }
+      }
+    }
+    if (!need_update)
+      return;
+
     data->most_visited_pages_.clear();
 
     for (size_t i = 0; i < urls.size() && i < kMostVisitedItems; i++) {