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

chrome/browser/win/jumplist.cc

 // Copyright 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/win/jumplist.h"
 
 #include "base/base_paths.h"
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/command_line.h"
@@ skipping 274 matching lines @@
   Terminate();
 }
 
 void JumpList::OnMostVisitedURLsAvailable(
     const history::MostVisitedURLList& urls) {
   DCHECK(CalledOnValidThread());
 
   {
     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++) {
       const history::MostVisitedURL& url = urls[i];
       scoped_refptr<ShellLinkItem> link = CreateShellLink();
       std::string url_string = url.url.spec();
       base::string16 url_string_wide = base::UTF8ToUTF16(url_string);
       link->GetCommandLine()->AppendArgNative(url_string_wide);
       link->GetCommandLine()->AppendSwitchASCII(
           switches::kWinJumplistAction, jumplist::kMostVisitedCategory);
@@ skipping 551 matching lines @@
           app_id, profile_dir, local_most_visited_pages,
           local_recently_closed_pages, most_visited_pages_have_updates,
           recently_closed_pages_have_updates, incognito_availability)) {
     base::AutoLock auto_lock(data->list_lock_);
     if (most_visited_pages_have_updates)
       data->most_visited_pages_have_updates_ = true;
     if (recently_closed_pages_have_updates)
       data->recently_closed_pages_have_updates_ = true;
   }
 }