Filter redundant JumpList favicons' fetching and related

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 74abc8c6c0affafb89b3d9280a6c4d94278756f1..60696366f5599ab1c7426e39f8e3c8873fc30fc8 100644
--- a/chrome/browser/win/jumplist.cc
+++ b/chrome/browser/win/jumplist.cc
@@ -467,11 +467,15 @@ void JumpList::OnMostVisitedURLsAvailable(
   // If we have a pending favicon request, cancel it here (it is out of date).
   CancelPendingUpdate();
 
+  // There're at most 9 JumpList items can be displayed for the "Most Visited"

[grt (UTC plus 2), 2017/05/03 10:51:48]

nit: "At most 9..."

[chengx, 2017/05/03 20:45:14]

Done.

+  // category.
+  const int kMostVistedCount = 9;
   {
     JumpListData* data = &jumplist_data_->data;
     base::AutoLock auto_lock(data->list_lock_);
     data->most_visited_pages_.clear();
-    for (size_t i = 0; i < urls.size(); i++) {
+
+    for (size_t i = 0; i < urls.size() && i < kMostVistedCount; i++) {
       const history::MostVisitedURL& url = urls[i];
       scoped_refptr<ShellLinkItem> link = CreateShellLink();
       std::string url_string = url.url.spec();
@@ -492,6 +496,21 @@ void JumpList::OnMostVisitedURLsAvailable(
 
 void JumpList::TabRestoreServiceChanged(sessions::TabRestoreService* service) {
   DCHECK(CalledOnValidThread());
+
+  // Initialize the one-shot timer to update the the "Recently Closed" category
+  // in a while. If there is already a request queued then cancel it and post
+  // the new request. This ensures that JumpList update of the "Recently Closed"
+  // category won't happen until there has been a brief quiet period, thus
+  // avoiding update storms.
+  if (timer_recently_closed_.IsRunning()) {
+    timer_recently_closed_.Reset();
+  } else {
+    timer_recently_closed_.Start(FROM_HERE, kDelayForJumplistUpdate, this,

[grt (UTC plus 2), 2017/05/03 10:51:48]

i suggest making the bind explicit so that you're not relying on a method in
BaseTimerMethodPointer (which is documented as an implementation detail):
base::Bind(&JumpList::DeferredTabRestoreServiceChanged, base::Unretained(this))

[chengx, 2017/05/03 20:45:14]

Done.

+                                 &JumpList::DeferredTabRestoreServiceChanged);
+  }
+}
+
+void JumpList::DeferredTabRestoreServiceChanged() {
   // if we have a pending favicon request, cancel it here (it is out of date).
   CancelPendingUpdate();
 
@@ -661,22 +680,6 @@ void JumpList::PostRunUpdate() {
   DCHECK(CalledOnValidThread());
 
   TRACE_EVENT0("browser", "JumpList::PostRunUpdate");
-  // Initialize the one-shot timer to update the jumplists in a while.
-  // If there is already a request queued then cancel it and post the new
-  // request. This ensures that JumpListUpdates won't happen until there has
-  // been a brief quiet period, thus avoiding update storms.
-  if (timer_.IsRunning()) {
-    timer_.Reset();
-  } else {
-    timer_.Start(FROM_HERE, kDelayForJumplistUpdate, this,
-                 &JumpList::DeferredRunUpdate);
-  }
-}
-
-void JumpList::DeferredRunUpdate() {
-  DCHECK(CalledOnValidThread());
-
-  TRACE_EVENT0("browser", "JumpList::DeferredRunUpdate");
   // Check if incognito windows (or normal windows) are disabled by policy.
   IncognitoModePrefs::Availability incognito_availability =
       profile_ ? IncognitoModePrefs::GetAvailability(profile_->GetPrefs())
@@ -708,6 +711,21 @@ void JumpList::TopSitesLoaded(history::TopSites* top_sites) {
 
 void JumpList::TopSitesChanged(history::TopSites* top_sites,
                                ChangeReason change_reason) {
+  // Initialize the one-shot timer to update the the "Most visited" category in

[grt (UTC plus 2), 2017/05/03 10:51:48]

CancelPendingUpdate here, or is there a reason to let it continue while waiting
for the quiet period to pass? same comment for
JumpList::TabRestoreServiceChanged.

[chengx, 2017/05/03 20:45:14]

Yes, it should be called here. I've made the updates. Thanks! 

+  // a while. If there is already a request queued then cancel it and post the
+  // new request. This ensures that JumpList update of the "Most visited"
+  // category won't happen until there has been a brief quiet period, thus
+  // avoiding update storms.
+  if (timer_most_visited_.IsRunning()) {
+    timer_most_visited_.Reset();
+  } else {
+    timer_most_visited_.Start(FROM_HERE, kDelayForJumplistUpdate,
+                              base::Bind(&JumpList::DeferredTopSitesChanged,
+                                         base::Unretained(this), top_sites));

[grt (UTC plus 2), 2017/05/03 10:51:48]

rather than holding a reference to TopSites through the call, i think it's
better to use TopSitesFactory::GetForProfile to get the instance anew in the
callback.

[chengx, 2017/05/03 20:45:14]

Done.

+  }
+}
+
+void JumpList::DeferredTopSitesChanged(history::TopSites* top_sites) {
   top_sites->GetMostVisitedURLs(
       base::Bind(&JumpList::OnMostVisitedURLsAvailable,
                  weak_ptr_factory_.GetWeakPtr()),