Filter redundant JumpList favicons' fetching and related

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 <utility>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
@@ skipping 421 matching lines @@
 }
 
 void JumpList::CancelPendingUpdate() {
   DCHECK(CalledOnValidThread());
   if (task_id_ != base::CancelableTaskTracker::kBadTaskId) {
     cancelable_task_tracker_.TryCancel(task_id_);
     task_id_ = base::CancelableTaskTracker::kBadTaskId;
   }
 }
 
 void JumpList::Terminate() {

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

stop the timers in here, no?

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

Done. They should be stopped.

   DCHECK(CalledOnValidThread());
   CancelPendingUpdate();
   if (profile_) {
     sessions::TabRestoreService* tab_restore_service =
         TabRestoreServiceFactory::GetForProfile(profile_);
     if (tab_restore_service)
       tab_restore_service->RemoveObserver(this);
     scoped_refptr<history::TopSites> top_sites =
         TopSitesFactory::GetForProfile(profile_);
     if (top_sites)
       top_sites->RemoveObserver(this);
     pref_change_registrar_.reset();
   }
   profile_ = NULL;
 }
 
 void JumpList::ShutdownOnUIThread() {
   DCHECK(CalledOnValidThread());
   Terminate();
 }
 
 void JumpList::OnMostVisitedURLsAvailable(
     const history::MostVisitedURLList& urls) {
   DCHECK(CalledOnValidThread());
   // 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();
       std::wstring url_string_wide = base::UTF8ToWide(url_string);
       link->GetCommandLine()->AppendArgNative(url_string_wide);
       link->GetCommandLine()->AppendSwitchASCII(
           switches::kWinJumplistAction, jumplist::kMostVisitedCategory);
       link->set_title(!url.title.empty() ? url.title : url_string_wide);
       data->most_visited_pages_.push_back(link);
       data->icon_urls_.push_back(std::make_pair(url_string, link));
     }
     data->most_visited_pages_have_updates_ = true;
   }
 
   // Send a query that retrieves the first favicon.
   StartLoadingFavicon();
 }
 
 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();
 
   // Create a list of ShellLinkItems from the "Recently Closed" pages.
   // As noted above, we create a ShellLinkItem objects with the following
   // parameters.
   // * arguments
   //   The last URL of the tab object.
   // * title
   //   The title of the last URL.
   // * icon
   //   An empty string. This value is to be updated in OnFaviconDataAvailable().
   // This code is copied from
   // RecentlyClosedTabsHandler::TabRestoreServiceChanged() to emulate it.
   const int kRecentlyClosedCount = 3;
   sessions::TabRestoreService* tab_restore_service =
       TabRestoreServiceFactory::GetForProfile(profile_);
 
   {
     JumpListData* data = &jumplist_data_->data;
     base::AutoLock auto_lock(data->list_lock_);
     data->recently_closed_pages_.clear();
 
     for (const auto& entry : tab_restore_service->entries()) {

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

should this loop terminate early if recently_closed_pages_ fills up?

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

Yes, it should. Early return is enabled in both AddTab() and AddWindow() if this
fill-up check passes. It should be enabled for this for-loop to save the efforts
even more. 

       switch (entry->type) {
         case sessions::TabRestoreService::TAB:
           AddTab(static_cast<const sessions::TabRestoreService::Tab&>(*entry),
                  kRecentlyClosedCount, data);
           break;
         case sessions::TabRestoreService::WINDOW:
           AddWindow(
               static_cast<const sessions::TabRestoreService::Window&>(*entry),
               kRecentlyClosedCount, data);
           break;
@@ skipping 125 matching lines @@
   // category changes, mark the flags so that icon files for those categories
   // won't be updated later on.
   if (!waiting_for_icons)
     PostRunUpdate();
 }
 
 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())
                : IncognitoModePrefs::ENABLED;
 
   // Post a task to update the JumpList, which consists of 1) delete old icons,
   // 2) create new icons, 3) notify the OS.
   update_jumplist_task_runner_->PostTask(
       FROM_HERE, base::Bind(&RunUpdateJumpList, incognito_availability, app_id_,
                             icon_dir_, base::RetainedRef(jumplist_data_)));
@@ skipping 11 matching lines @@
   delete_jumplisticons_task_runner_->PostTask(
       FROM_HERE,
       base::Bind(&DeleteDirectory, std::move(icon_dir_old), kFileDeleteLimit));
 }
 
 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()),
       false);
 }