Fix stability and data racing issues, coalesce more updates for JumpList

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 281ea908aac175a6faee31b360ccbd9be343e251..68358555dd2f6b5a33951fbd24d6751ee6721553 100644
--- a/chrome/browser/win/jumplist.cc
+++ b/chrome/browser/win/jumplist.cc
@@ -53,7 +53,6 @@
 #include "url/gurl.h"
 
 using content::BrowserThread;
-using JumpListData = JumpList::JumpListData;
 
 namespace {
 
@@ -196,15 +195,14 @@ base::FilePath GenerateJumplistIconDirName(
 
 }  // namespace
 
-JumpList::JumpListData::JumpListData() {}
+JumpList::JumpListUpdateResults::JumpListUpdateResults() {}
 
-JumpList::JumpListData::~JumpListData() {}
+JumpList::JumpListUpdateResults::~JumpListUpdateResults() {}
 
 JumpList::JumpList(Profile* profile)
     : RefcountedKeyedService(content::BrowserThread::GetTaskRunnerForThread(
           content::BrowserThread::UI)),
       profile_(profile),
-      jumplist_data_(new base::RefCountedData<JumpListData>),
       task_id_(base::CancelableTaskTracker::kBadTaskId),
       update_jumplist_task_runner_(base::CreateCOMSTATaskRunnerWithTraits(
           {base::MayBlock(), base::TaskPriority::USER_VISIBLE,
@@ -230,12 +228,13 @@ JumpList::JumpList(Profile* profile)
 
   scoped_refptr<history::TopSites> top_sites =

[grt (UTC plus 2), 2017/06/09 10:42:57]

nit: move this down just after the comment below with no blank lines between:
  // The comment
  scoped_refptr<>... = ...;
  if (top_sites)
    ...;

[chengx, 2017/06/10 05:56:17]

Done.

       TopSitesFactory::GetForProfile(profile_);
-  if (top_sites) {
-    // Register as TopSitesObserver so that we can update ourselves when the
-    // TopSites changes. TopSites updates itself after a delay. This is
-    // especially noticable when your profile is empty.
+
+  // Register as TopSitesObserver so that we can update ourselves when the
+  // TopSites changes. TopSites updates itself after a delay. This is especially
+  // noticable when your profile is empty.
+  if (top_sites)
     top_sites->AddObserver(this);
-  }
+
   tab_restore_service->AddObserver(this);

[grt (UTC plus 2), 2017/06/09 10:42:57]

please add a doc comment explaining why the TRS is being observed.

[chengx, 2017/06/10 05:56:17]

Done.

   pref_change_registrar_.reset(new PrefChangeRegistrar);

[grt (UTC plus 2), 2017/06/09 10:42:56]

please add a doc comment explaining why kIncognitoModeAvailability is being
monitored for changes.

[chengx, 2017/06/10 05:56:17]

Done.

   pref_change_registrar_->Init(profile_->GetPrefs());
@@ -264,8 +263,7 @@ void JumpList::CancelPendingUpdate() {
 
 void JumpList::Terminate() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  timer_most_visited_.Stop();
-  timer_recently_closed_.Stop();
+  timer_.Stop();
   CancelPendingUpdate();
   if (profile_) {

[grt (UTC plus 2), 2017/06/09 10:42:56]

  update_in_progress_ = false;
just before this line

[chengx, 2017/06/10 05:56:16]

Done.

     sessions::TabRestoreService* tab_restore_service =
@@ -290,35 +288,29 @@ void JumpList::OnMostVisitedURLsAvailable(
     const history::MostVisitedURLList& urls) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
-  {
-    JumpListData* data = &jumplist_data_->data;
-    base::AutoLock auto_lock(data->list_lock_);
-
-    // There is no need to update the JumpList if the top most visited sites in
-    // display have not changed.
-    if (MostVisitedItemsUnchanged(data->most_visited_pages_, urls,
-                                  kMostVisitedItems)) {
-      return;
-    }
+  // There is no need to update the JumpList if the top most visited sites in
+  // display have not changed.
+  if (MostVisitedItemsUnchanged(most_visited_pages_, urls, kMostVisitedItems))
+    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);
-      link->set_title(!url.title.empty() ? url.title : url_string_wide);
-      link->set_url(url_string);
-      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;
+  most_visited_pages_.clear();
+

[grt (UTC plus 2), 2017/06/09 10:42:56]

  const size_t num_items = std::min(urls.size(), kMostVisitedItems);
  most_visited_pages_.reserve(num_items);
  icon_urls_.reserve(icon_urls_.size() + num_items);

[chengx, 2017/06/10 05:56:17]

I introduced num_items to avoid duplicated value comparison in the for-loop.
However, most_visited_pages_ and icon_urls_ are std::list rather than
std::vector, so reserve() is not applicable here. 

[grt (UTC plus 2), 2017/06/12 07:15:32]

:-)

[chengx, 2017/06/12 22:05:28]

Acknowledged.

+  for (size_t i = 0; i < urls.size() && i < kMostVisitedItems; i++) {

[grt (UTC plus 2), 2017/06/09 10:42:56]

...; i < num_items; ++i) {

(note: always prefer pre-increment)

[chengx, 2017/06/10 05:56:17]

Done.

+    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);
+    link->set_title(!url.title.empty() ? url.title : url_string_wide);
+    link->set_url(url_string);
+    most_visited_pages_.push_back(link);
+    icon_urls_.push_back(std::make_pair(url_string, link));

[grt (UTC plus 2), 2017/06/09 10:42:57]

  icon_urls_.emplace_back(std::move(url_string), std::move(link));

[chengx, 2017/06/10 05:56:17]

Done.

   }
 
+  most_visited_pages_have_updates_ = true;
+
   // Send a query that retrieves the first favicon.
   StartLoadingFavicon();
 }
@@ -326,21 +318,24 @@ void JumpList::OnMostVisitedURLsAvailable(
 void JumpList::TabRestoreServiceChanged(sessions::TabRestoreService* service) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
-  // if we have a pending favicon request, cancel it here (it is out of date).
+  recently_closed_has_pending_notification_ = true;
+
+  if (update_in_progress_)

[grt (UTC plus 2), 2017/06/09 10:42:56]

please document this. for example:
  // Postpone handling this notification until a pending update completes.

and put the same comment in TopSitesChanged

[chengx, 2017/06/10 05:56:16]

Done.

+    return;
+
+  // if we have a pending favicon request, cancel it here as it's out of date.
   CancelPendingUpdate();
 
-  // 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();
+  // Initialize the one-shot timer to update the JumpList in a while. If there
+  // is already a request queued then cancel it and post the new request. This
+  // ensures that JumpList update won't happen until there has been a brief
+  // quiet period, thus avoiding update storms.
+  if (timer_.IsRunning()) {

[grt (UTC plus 2), 2017/06/09 10:42:57]

this block is repeated three times. please pull it out into its own function.
ResetQuietPeriodTimer or something like that would have a nice ring to it.

[chengx, 2017/06/10 05:56:18]

I have added a new InitializeTimerForUpdate method and put everything in it. 

+    timer_.Reset();
   } else {
-    timer_recently_closed_.Start(
+    timer_.Start(
         FROM_HERE, kDelayForJumplistUpdate,
-        base::Bind(&JumpList::DeferredTabRestoreServiceChanged,
-                   base::Unretained(this)));
+        base::Bind(&JumpList::DeferredChanged, base::Unretained(this)));

[grt (UTC plus 2), 2017/06/09 10:42:57]

please add a comment such as:
  // base::Unretained is safe since |this| is guaranteed to outlive timer_.

[chengx, 2017/06/10 05:56:17]

Done.

   }
 }
 
@@ -348,13 +343,12 @@ void JumpList::TabRestoreServiceDestroyed(
     sessions::TabRestoreService* service) {}
 
 bool JumpList::AddTab(const sessions::TabRestoreService::Tab& tab,
-                      size_t max_items,
-                      JumpListData* data) {
+                      size_t max_items) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  data->list_lock_.AssertAcquired();
 
-  // This code adds the URL and the title strings of the given tab to |data|.
-  if (data->recently_closed_pages_.size() >= max_items)
+  // This code adds the URL and the title strings of the given tab to the
+  // JumpList variables.
+  if (recently_closed_pages_.size() >= max_items)
     return false;
 
   scoped_refptr<ShellLinkItem> link = CreateShellLink();
@@ -366,24 +360,22 @@ bool JumpList::AddTab(const sessions::TabRestoreService::Tab& tab,
                                             jumplist::kRecentlyClosedCategory);
   link->set_title(current_navigation.title());
   link->set_url(url);
-  data->recently_closed_pages_.push_back(link);
-  data->icon_urls_.push_back(std::make_pair(std::move(url), std::move(link)));
+  recently_closed_pages_.push_back(link);
+  icon_urls_.push_back(std::make_pair(std::move(url), std::move(link)));

[grt (UTC plus 2), 2017/06/09 10:42:56]

  icon_urls_.emplace_back(std::move(url), std::move(link));

[chengx, 2017/06/10 05:56:17]

Done.

 
   return true;
 }
 
 void JumpList::AddWindow(const sessions::TabRestoreService::Window& window,
-                         size_t max_items,
-                         JumpListData* data) {
+                         size_t max_items) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  data->list_lock_.AssertAcquired();
 
   // This code enumerates all the tabs in the given window object and add their

[grt (UTC plus 2), 2017/06/09 10:42:57]

this comment adds nothing over the doc comment in the .h and the code below.
please remove it.

[chengx, 2017/06/10 05:56:17]

Done.

-  // URLs and titles to |data|.
+  // URLs and titles to the JumpList variables.
   DCHECK(!window.tabs.empty());
 
   for (const auto& tab : window.tabs) {
-    if (!AddTab(*tab, max_items, data))
+    if (!AddTab(*tab, max_items))
       return;
   }
 }
@@ -394,16 +386,12 @@ void JumpList::StartLoadingFavicon() {
   base::ElapsedTimer timer;

[grt (UTC plus 2), 2017/06/09 10:42:57]

do the early return before this line:

  if (icon_urls_.empty()) {
    // No more favicons are needed by the application JumpList. Schedule an
    // update on a background thread.
    PostRunUpdate();
    return;
  }

then the rest of the code is a bit simpler:
  base::ElapsedTimer timer;
  GURL url = GURL(icon_urls_.front().first);
  ...

[chengx, 2017/06/10 05:56:18]

Done.

 
   GURL url;
-  bool waiting_for_icons = true;
-  {
-    JumpListData* data = &jumplist_data_->data;
-    base::AutoLock auto_lock(data->list_lock_);
-    waiting_for_icons = !data->icon_urls_.empty();
-    if (waiting_for_icons) {
-      // Ask FaviconService if it has a favicon of a URL.
-      // When FaviconService has one, it will call OnFaviconDataAvailable().
-      url = GURL(data->icon_urls_.front().first);
-    }
+
+  bool waiting_for_icons = !icon_urls_.empty();
+  if (waiting_for_icons) {
+    // Ask FaviconService if it has a favicon of a URL.
+    // When FaviconService has one, it will call OnFaviconDataAvailable().
+    url = GURL(icon_urls_.front().first);
   }
 
   if (!waiting_for_icons) {
@@ -435,25 +423,20 @@ void JumpList::OnFaviconDataAvailable(
   // If there is currently a favicon request in progress, it is now outdated,
   // as we have received another, so nullify the handle from the old request.
   task_id_ = base::CancelableTaskTracker::kBadTaskId;
-  // Lock the list to set icon data and pop the url.
-  {
-    JumpListData* data = &jumplist_data_->data;
-    base::AutoLock auto_lock(data->list_lock_);
-    // Attach the received data to the ShellLinkItem object.
-    // This data will be decoded by the RunUpdateJumpList
-    // method.
-    if (!image_result.image.IsEmpty() && !data->icon_urls_.empty() &&
-        data->icon_urls_.front().second.get()) {
-      gfx::ImageSkia image_skia = image_result.image.AsImageSkia();
-      image_skia.EnsureRepsForSupportedScales();
-      std::unique_ptr<gfx::ImageSkia> deep_copy(image_skia.DeepCopy());
-      data->icon_urls_.front().second->set_icon_image(*deep_copy);
-    }
 
-    if (!data->icon_urls_.empty())
-      data->icon_urls_.pop_front();
+  // Attach the received data to the ShellLinkItem object. This data will be
+  // decoded by the RunUpdateJumpList method.
+  if (!image_result.image.IsEmpty() && !icon_urls_.empty() &&
+      icon_urls_.front().second.get()) {
+    gfx::ImageSkia image_skia = image_result.image.AsImageSkia();
+    image_skia.EnsureRepsForSupportedScales();
+    std::unique_ptr<gfx::ImageSkia> deep_copy(image_skia.DeepCopy());
+    icon_urls_.front().second->set_icon_image(*deep_copy);
   }
 
+  if (!icon_urls_.empty())

[grt (UTC plus 2), 2017/06/09 10:42:55]

this condition is checked twice. how about:
  if (!icon_urls_.empty()) {
    if (!image_result.image.IsEmpty() && icon_urls_.front().second.get()) {
      ...
    }
    icon_urls_.pop_front();
  }

[chengx, 2017/06/10 05:56:17]

Agreed this is the right thing to do. Done.

+    icon_urls_.pop_front();
+
   // TODO(chengx): Remove the UMA histogram after fixing http://crbug.com/717236
   UMA_HISTOGRAM_TIMES("WinJumplist.OnFaviconDataAvailableDuration",
                       timer.Elapsed());
@@ -465,12 +448,7 @@ void JumpList::OnFaviconDataAvailable(
 void JumpList::OnIncognitoAvailabilityChanged() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
-  bool waiting_for_icons = true;
-  {
-    JumpListData* data = &jumplist_data_->data;
-    base::AutoLock auto_lock(data->list_lock_);
-    waiting_for_icons = !data->icon_urls_.empty();
-  }
+  bool waiting_for_icons = !icon_urls_.empty();
 
   // Since neither the "Most Visited" category nor the "Recently Closed"
   // category changes, mark the flags so that icon files for those categories
@@ -486,33 +464,49 @@ void JumpList::PostRunUpdate() {
   if (!profile_)

[grt (UTC plus 2), 2017/06/09 10:42:55]

can this ever be true? as long as Terminate() really terminates everything, i'm
not sure how it could be. please remove it unless you can determine an odd
sequence of events that could lead to it being true.

[chengx, 2017/06/10 05:56:17]

I've removed it.

     return;
 
+  update_in_progress_ = true;
+
   base::FilePath profile_dir = profile_->GetPath();
 
   // Check if incognito windows (or normal windows) are disabled by policy.
   IncognitoModePrefs::Availability incognito_availability =
       IncognitoModePrefs::GetAvailability(profile_->GetPrefs());
 
-  // 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(
+  // Make local copies of JumpList member variables and use them for an update.
+  ShellLinkItemList local_most_visited_pages = most_visited_pages_;
+  ShellLinkItemList local_recently_closed_pages = recently_closed_pages_;
+
+  bool most_visited_pages_have_updates = most_visited_pages_have_updates_;
+  bool recently_closed_pages_have_updates = recently_closed_pages_have_updates_;
+
+  JumpListUpdateResults* update_results = new JumpListUpdateResults();

[grt (UTC plus 2), 2017/06/09 10:42:56]

  auto update_results = base::MakeUnique<JumpListUpdateResults>();
and std::move it as needed below.

[chengx, 2017/06/10 05:56:17]

Done.

+  update_results->most_visited_icons_in_update_ = most_visited_icons_;
+  update_results->recently_closed_icons_in_update = recently_closed_icons_;
+
+  // Post a task to update the JumpList, which consists of 1) create new icons,
+  // 2) delete old icons, 3) notify the OS.
+  update_jumplist_task_runner_->PostTaskAndReply(

[grt (UTC plus 2), 2017/06/09 10:42:56]

PTAR returns a bool indicating whether or not the task may run at some point in
the future. to handle this case, i suggest:
  if (!update_jumplist_task_runner_->PostTaskAndReply(...)) {
    OnRunUpdateCompletion(base::MakeUnique<JumpListUpdateResults>());
  }
so that the instance resets its state properly.

[chengx, 2017/06/10 05:56:16]

Thanks for the reminder! This is cool!

       FROM_HERE,
-      base::Bind(&JumpList::RunUpdateJumpList, this, incognito_availability,
-                 app_id_, profile_dir, base::RetainedRef(jumplist_data_)));
+      base::Bind(&JumpList::RunUpdateJumpList, app_id_, profile_dir,
+                 local_most_visited_pages, local_recently_closed_pages,
+                 most_visited_pages_have_updates,
+                 recently_closed_pages_have_updates, incognito_availability,
+                 update_results),
+      base::Bind(&JumpList::OnRunUpdateCompletion, base::Unretained(this),

[grt (UTC plus 2), 2017/06/09 10:42:56]

base::Unretained -> weak_ptr_factory_.GetWeakPtr()

[chengx, 2017/06/10 05:56:17]

Done.

+                 base::Passed(base::WrapUnique(update_results))));
+
+  // Post a task to delete folders JumpListIcons and JumpListIconsOld as they

[grt (UTC plus 2), 2017/06/09 10:42:56]

wdyt of moving this stuff down into OnRunUpdateCompletion so that it doesn't
contend with the update?

[grt (UTC plus 2), 2017/06/09 10:42:57]

nit: "...to delete the Foo and Bar folders as they..."

[chengx, 2017/06/10 05:56:17]

Comments updated.

[chengx, 2017/06/10 05:56:17]

SGTM. Done.

+  // are no longer needed. Now we have folders JumpListIcons{MostVisited,
+  // RecentClosed} instead.
 
-  // Post a task to delete JumpListIcons folder as it's no longer needed.
-  // Now we have JumpListIconsMostVisited folder and JumpListIconsRecentClosed
-  // folder instead.
   base::FilePath icon_dir =
       GenerateJumplistIconDirName(profile_dir, FILE_PATH_LITERAL(""));
-
   delete_jumplisticons_task_runner_->PostTask(
       FROM_HERE,
       base::Bind(&DeleteDirectory, std::move(icon_dir), kFileDeleteLimit));
 
-  // Post a task to delete JumpListIconsOld folder as it's no longer needed.
   base::FilePath icon_dir_old =
       GenerateJumplistIconDirName(profile_dir, FILE_PATH_LITERAL("Old"));
-
   delete_jumplisticons_task_runner_->PostTask(
       FROM_HERE,
       base::Bind(&DeleteDirectory, std::move(icon_dir_old), kFileDeleteLimit));
@@ -523,31 +517,53 @@ void JumpList::TopSitesLoaded(history::TopSites* top_sites) {
 
 void JumpList::TopSitesChanged(history::TopSites* top_sites,
                                ChangeReason change_reason) {
-  // If we have a pending favicon request, cancel it here (it is out of date).
+  most_visited_has_pending_notification_ = true;

[grt (UTC plus 2), 2017/06/09 10:42:56]

"Most Visited" is the name of the jumplist section, whereas "Top Sites" is the
thing in the browser that is being observed. I would rename this variable to
top_sites_has_pending_notification_ or top_sites_updated_ (short and sweet). if
you like that, please change recently_closed_has_pending_notification_ to
recently_closed_updated_, too.

[grt (UTC plus 2), 2017/06/09 10:42:56]

  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);

[chengx, 2017/06/10 05:56:17]

Thanks for the suggestion. I prefer top_sites_has_pending_notification_ though,
because there is already a variable called most_visited_should_update_. TopSites
having notifications doesn't mean most-visited category needs to be updated,
because TopSites cares about the top 24 urls, while the most-visited category
only cares about the top 5. I do need these two sets of flags for different
purposes. I've also renamed other variables accordingly.

[chengx, 2017/06/10 05:56:18]

DCHECK added.

+
+  if (update_in_progress_)
+    return;
+
+  // If we have a pending favicon request, cancel it here as it's out of date.
   CancelPendingUpdate();
 
-  // Initialize the one-shot timer to update the the "Most visited" 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 "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();
+  // Initialize the one-shot timer to update the JumpList in a while. If there
+  // is already a request queued then cancel it and post the new request. This
+  // ensures that JumpList update won't happen until there has been a brief
+  // quiet period, thus avoiding update storms.
+  if (timer_.IsRunning()) {
+    timer_.Reset();
   } else {
-    timer_most_visited_.Start(
+    timer_.Start(
         FROM_HERE, kDelayForJumplistUpdate,
-        base::Bind(&JumpList::DeferredTopSitesChanged, base::Unretained(this)));
+        base::Bind(&JumpList::DeferredChanged, base::Unretained(this)));
   }
 }
 
-void JumpList::DeferredTopSitesChanged() {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-
+void JumpList::DeferredChanged() {
   if (updates_to_skip_ > 0) {

[grt (UTC plus 2), 2017/06/09 10:42:57]

  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);

[chengx, 2017/06/10 05:56:16]

Done.

     --updates_to_skip_;
     return;
   }
 
+  // Retrieve the recently closed URLs synchronously.
+  if (recently_closed_has_pending_notification_) {
+    recently_closed_has_pending_notification_ = false;
+    DeferredTabRestoreServiceChanged();
+  }
+
+  // If TopSites has updates, retrieve the URLs asynchronously, and on its
+  // completion, trigger favicon loading.
+  // Otherwise, call StartLoadingFavicon directly to start favicon loading.
+  if (most_visited_has_pending_notification_) {
+    most_visited_has_pending_notification_ = false;
+    DeferredTopSitesChanged();
+  } else {
+    StartLoadingFavicon();
+  }
+}
+
+void JumpList::DeferredTopSitesChanged() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
   // Opening the first tab in one session triggers a TopSite history sync.
   // Delay this sync till the first tab is closed to allow the "recently closed"
   // category from last session to stay longer.
@@ -567,20 +583,6 @@ void JumpList::DeferredTopSitesChanged() {
 void JumpList::DeferredTabRestoreServiceChanged() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
-  if (updates_to_skip_ > 0) {
-    --updates_to_skip_;
-    return;
-  }
-
-  // Force a TopSite history sync when closing a first tab in one session.
-  if (!has_tab_closed_) {
-    has_tab_closed_ = true;
-    scoped_refptr<history::TopSites> top_sites =
-        TopSitesFactory::GetForProfile(profile_);
-    if (top_sites)
-      top_sites->SyncWithHistory();
-  }
-
   // Create a list of ShellLinkItems from the "Recently Closed" pages.
   // As noted above, we create a ShellLinkItem objects with the following
   // parameters.
@@ -594,85 +596,66 @@ void JumpList::DeferredTabRestoreServiceChanged() {
   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();
+  recently_closed_pages_.clear();
 
-    for (const auto& entry : tab_restore_service->entries()) {
-      if (data->recently_closed_pages_.size() >= kRecentlyClosedItems)
+  for (const auto& entry : tab_restore_service->entries()) {
+    if (recently_closed_pages_.size() >= kRecentlyClosedItems)
+      break;
+    switch (entry->type) {
+      case sessions::TabRestoreService::TAB:
+        AddTab(static_cast<const sessions::TabRestoreService::Tab&>(*entry),
+               kRecentlyClosedItems);
+        break;
+      case sessions::TabRestoreService::WINDOW:
+        AddWindow(
+            static_cast<const sessions::TabRestoreService::Window&>(*entry),
+            kRecentlyClosedItems);
         break;
-      switch (entry->type) {
-        case sessions::TabRestoreService::TAB:
-          AddTab(static_cast<const sessions::TabRestoreService::Tab&>(*entry),
-                 kRecentlyClosedItems, data);
-          break;
-        case sessions::TabRestoreService::WINDOW:
-          AddWindow(
-              static_cast<const sessions::TabRestoreService::Window&>(*entry),
-              kRecentlyClosedItems, data);
-          break;
-      }
     }
-
-    data->recently_closed_pages_have_updates_ = true;
   }
 
-  // Send a query that retrieves the first favicon.
-  StartLoadingFavicon();
-}
+  recently_closed_pages_have_updates_ = true;
 
-void JumpList::DeleteIconFiles(const base::FilePath& icon_dir,
-                               JumpListCategory category) {
-  base::flat_map<std::string, base::FilePath>* source_map = nullptr;
-  switch (category) {
-    case JumpListCategory::kMostVisited:
-      source_map = &most_visited_icons_;
-      break;
-    case JumpListCategory::kRecentlyClosed:
-      source_map = &recently_closed_icons_;
-      break;
+  // Force a TopSite history sync when closing a first tab in one session.
+  if (!has_tab_closed_) {
+    has_tab_closed_ = true;
+    scoped_refptr<history::TopSites> top_sites =
+        TopSitesFactory::GetForProfile(profile_);
+    if (top_sites)
+      top_sites->SyncWithHistory();
   }
+}
 
+// static
+void JumpList::DeleteIconFiles(const base::FilePath& icon_dir,
+                               URLIconCache* icon_cache) {
   // Put all cached icon file paths into a set.
   base::flat_set<base::FilePath> cached_files;
-  cached_files.reserve(source_map->size());
+  cached_files.reserve(icon_cache->size());
 
-  for (const auto& url_path_pair : *source_map)
+  for (const auto& url_path_pair : *icon_cache)
     cached_files.insert(url_path_pair.second);
 
   DeleteNonCachedFiles(icon_dir, cached_files);
 }
 
+// static
 int JumpList::CreateIconFiles(const base::FilePath& icon_dir,
                               const ShellLinkItemList& item_list,
                               size_t max_items,
-                              JumpListCategory category) {
+                              URLIconCache* icon_cache) {
   // TODO(chengx): Remove the UMA histogram after fixing http://crbug.com/40407.
   SCOPED_UMA_HISTOGRAM_TIMER("WinJumplist.CreateIconFilesDuration");
 
   int icons_created = 0;
 
-  // Reuse icons for urls that were already present in the jumplist for this
-  // category.
-
-  base::flat_map<std::string, base::FilePath>* source_map = nullptr;
-  switch (category) {
-    case JumpListCategory::kMostVisited:
-      source_map = &most_visited_icons_;
-      break;
-    case JumpListCategory::kRecentlyClosed:
-      source_map = &recently_closed_icons_;
-      break;
-  }
-
-  base::flat_map<std::string, base::FilePath> updated_map;
-
+  // Reuse icons for urls that already present in the current JumpList.
+  URLIconCache updated_map;
   for (ShellLinkItemList::const_iterator iter = item_list.begin();
        iter != item_list.end() && max_items > 0; ++iter, --max_items) {
     ShellLinkItem* item = iter->get();
-    auto cache_iter = source_map->find(item->url());
-    if (cache_iter != source_map->end()) {
+    auto cache_iter = icon_cache->find(item->url());
+    if (cache_iter != icon_cache->end()) {
       item->set_icon(cache_iter->second.value(), 0);
       updated_map[item->url()] = cache_iter->second;
     } else {
@@ -684,74 +667,70 @@ int JumpList::CreateIconFiles(const base::FilePath& icon_dir,
       }
     }
   }
-  source_map->swap(updated_map);
+  icon_cache->swap(updated_map);
 
   return icons_created;
 }
 
+// static
 int JumpList::UpdateIconFiles(const base::FilePath& icon_dir,
                               const ShellLinkItemList& page_list,
                               size_t slot_limit,
-                              JumpListCategory category) {
+                              URLIconCache* icon_cache) {
   int icons_created = 0;
 
-  // Maximum number of icon files that each JumpList icon folder may hold, which
-  // is set to 2 times the normal amount.
-  size_t icon_limit =
-      2 * ((category == JumpListCategory::kMostVisited) ? kMostVisitedItems
-                                                        : kRecentlyClosedItems);
-
   // Clear the JumpList icon folder at |icon_dir| and the cache when
-  // 1) "Most visited" category updates for the 1st time after Chrome is
-  //     launched. This actually happens right after Chrome is launched.
-  // 2) "Recently closed" category updates for the 1st time after Chrome is
-  //     launched.
-  // 3) The number of icons in |icon_dir| has exceeded the limit.
-  if ((category == JumpListCategory::kMostVisited &&
-       most_visited_icons_.empty()) ||
-      (category == JumpListCategory::kRecentlyClosed &&
-       recently_closed_icons_.empty()) ||
-      FilesExceedLimitInDir(icon_dir, icon_limit)) {
+  // 1) |icon_cache| is empty. This happens when "Most visited" or "Recently
+  //    closed" category updates for the 1st time after Chrome is launched.
+  // 2) The number of icons in |icon_dir| has exceeded the limit.
+  if (icon_cache->empty() || FilesExceedLimitInDir(icon_dir, slot_limit * 2)) {
     DeleteDirectoryContentAndLogRuntime(icon_dir, kFileDeleteLimit);
-    most_visited_icons_.clear();
-    recently_closed_icons_.clear();
+    icon_cache->clear();
     // Create new icons only when the directory exists and is empty.
     if (base::CreateDirectory(icon_dir) && base::IsDirectoryEmpty(icon_dir))
       icons_created +=
-          CreateIconFiles(icon_dir, page_list, slot_limit, category);
+          CreateIconFiles(icon_dir, page_list, slot_limit, icon_cache);
   } else if (base::CreateDirectory(icon_dir)) {
-    icons_created += CreateIconFiles(icon_dir, page_list, slot_limit, category);
-    DeleteIconFiles(icon_dir, category);
+    icons_created +=
+        CreateIconFiles(icon_dir, page_list, slot_limit, icon_cache);
+    DeleteIconFiles(icon_dir, icon_cache);
   }
 
   return icons_created;
 }
 
-bool JumpList::UpdateJumpList(
+// static
+void JumpList::RunUpdateJumpList(
     const base::string16& app_id,
     const base::FilePath& profile_dir,
     const ShellLinkItemList& most_visited_pages,
     const ShellLinkItemList& recently_closed_pages,
     bool most_visited_pages_have_updates,
     bool recently_closed_pages_have_updates,
-    IncognitoModePrefs::Availability incognito_availability) {
+    IncognitoModePrefs::Availability incognito_availability,
+    JumpListUpdateResults* update_results) {
   if (!JumpListUpdater::IsEnabled())
-    return true;
+    return;
+
+  DCHECK(update_results);
 
   JumpListUpdater jumplist_updater(app_id);
 
   base::ElapsedTimer begin_update_timer;
 
-  if (!jumplist_updater.BeginUpdate())
-    return false;
+  if (!jumplist_updater.BeginUpdate()) {
+    update_results->update_success_ = false;
+    return;
+  }
 
   // Discard this JumpList update if JumpListUpdater::BeginUpdate takes longer
   // than the maximum allowed time, as it's very likely the following update
   // steps will also take a long time. As we've not updated the icons on the
   // disk, discarding this update wont't affect the current JumpList used by OS.
   if (begin_update_timer.Elapsed() >= kTimeOutForJumplistBeginUpdate) {
-    updates_to_skip_ = kUpdatesToSkipUnderHeavyLoad;
-    return false;
+    update_results->update_success_ = false;
+    update_results->update_timeout_ = true;
+    return;
   }
 
   // Record the desired number of icons created in this JumpList update.
@@ -762,9 +741,9 @@ bool JumpList::UpdateJumpList(
     base::FilePath icon_dir_most_visited = GenerateJumplistIconDirName(
         profile_dir, FILE_PATH_LITERAL("MostVisited"));
 
-    icons_created +=
-        UpdateIconFiles(icon_dir_most_visited, most_visited_pages,
-                        kMostVisitedItems, JumpListCategory::kMostVisited);
+    icons_created += UpdateIconFiles(
+        icon_dir_most_visited, most_visited_pages, kMostVisitedItems,
+        &update_results->most_visited_icons_in_update_);
   }
 
   // Update the icons for "Recently Closed" category of the JumpList if needed.
@@ -774,7 +753,7 @@ bool JumpList::UpdateJumpList(
 
     icons_created += UpdateIconFiles(
         icon_dir_recent_closed, recently_closed_pages, kRecentlyClosedItems,
-        JumpListCategory::kRecentlyClosed);
+        &update_results->recently_closed_icons_in_update);
   }
 
   // TODO(chengx): Remove the UMA histogram after fixing http://crbug.com/40407.
@@ -791,14 +770,16 @@ bool JumpList::UpdateJumpList(
   if (!jumplist_updater.AddCustomCategory(
           l10n_util::GetStringUTF16(IDS_NEW_TAB_MOST_VISITED),
           most_visited_pages, kMostVisitedItems)) {
-    return false;
+    update_results->update_success_ = false;
+    return;
   }
 
   // Update the "Recently Closed" category of the JumpList.
   if (!jumplist_updater.AddCustomCategory(
           l10n_util::GetStringUTF16(IDS_RECENTLY_CLOSED), recently_closed_pages,
           kRecentlyClosedItems)) {
-    return false;
+    update_results->update_success_ = false;
+    return;
   }
 
   // If JumpListUpdater::AddCustomCategory or JumpListUpdater::CommitUpdate
@@ -810,68 +791,57 @@ bool JumpList::UpdateJumpList(
   // the JumpList panel is used instead, which doesn't look nice.
 
   if (add_custom_category_timer.Elapsed() >= kTimeOutForAddCustomCategory)
-    updates_to_skip_ = kUpdatesToSkipUnderHeavyLoad;
+    update_results->update_timeout_ = true;
 
   // Update the "Tasks" category of the JumpList.
-  if (!UpdateTaskCategory(&jumplist_updater, incognito_availability))
-    return false;
+  if (!UpdateTaskCategory(&jumplist_updater, incognito_availability)) {
+    update_results->update_success_ = false;
+    return;
+  }
 
   base::ElapsedTimer commit_update_timer;
 
   // Commit this transaction and send the updated JumpList to Windows.
-  bool commit_result = jumplist_updater.CommitUpdate();
+  if (!jumplist_updater.CommitUpdate())
+    update_results->update_success_ = false;
 
   if (commit_update_timer.Elapsed() >= kTimeOutForJumplistCommitUpdate)
-    updates_to_skip_ = kUpdatesToSkipUnderHeavyLoad;
+    update_results->update_timeout_ = true;
 
-  return commit_result;
+  return;

[grt (UTC plus 2), 2017/06/09 10:42:57]

omit

[chengx, 2017/06/10 05:56:17]

Done.

 }
 
-void JumpList::RunUpdateJumpList(
-    IncognitoModePrefs::Availability incognito_availability,
-    const base::string16& app_id,
-    const base::FilePath& profile_dir,
-    base::RefCountedData<JumpListData>* ref_counted_data) {
-  JumpListData* data = &ref_counted_data->data;
-  ShellLinkItemList local_most_visited_pages;
-  ShellLinkItemList local_recently_closed_pages;
-  bool most_visited_pages_have_updates;
-  bool recently_closed_pages_have_updates;
-
-  {
-    base::AutoLock auto_lock(data->list_lock_);
-    // Make sure we are not out of date: if icon_urls_ is not empty, then
-    // another notification has been received since we processed this one
-    if (!data->icon_urls_.empty())
-      return;
-
-    // Make local copies of lists and flags so we can release the lock.
-    local_most_visited_pages = data->most_visited_pages_;
-    local_recently_closed_pages = data->recently_closed_pages_;
+void JumpList::OnRunUpdateCompletion(
+    std::unique_ptr<JumpListUpdateResults> update_results) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
-    most_visited_pages_have_updates = data->most_visited_pages_have_updates_;
-    recently_closed_pages_have_updates =
-        data->recently_closed_pages_have_updates_;
+  // Update JumpList member variables based on the results from the update run
+  // just finished.
+  if (update_results->update_timeout_)
+    updates_to_skip_ = kUpdatesToSkipUnderHeavyLoad;
 
-    // Clear the flags to reflect that we'll take actions on these updates.
-    data->most_visited_pages_have_updates_ = false;
-    data->recently_closed_pages_have_updates_ = false;
+  if (update_results->update_success_) {
+    most_visited_icons_.swap(update_results->most_visited_icons_in_update_);
+    recently_closed_icons_.swap(
+        update_results->recently_closed_icons_in_update);
+    most_visited_pages_have_updates_ = false;
+    recently_closed_pages_have_updates_ = false;
   }
 
-  if (!most_visited_pages_have_updates && !recently_closed_pages_have_updates)
-    return;
+  update_in_progress_ = false;
 
-  // Update the application JumpList. If it fails, reset the flags to true if
-  // they were so that the corresponding JumpList categories will be tried to
-  // update again in the next run.
-  if (!UpdateJumpList(
-          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;
+  // Start another JumpList update if there is any new notification during the
+  // update run just finished.
+  if (most_visited_has_pending_notification_ ||
+      recently_closed_has_pending_notification_) {
+    if (timer_.IsRunning()) {
+      timer_.Reset();
+    } else {
+      timer_.Start(
+          FROM_HERE, kDelayForJumplistUpdate,
+          base::Bind(&JumpList::DeferredChanged, base::Unretained(this)));
+    }
   }
+
+  return;
 }