Reorder the methods in JumpList class according to the update workflow

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 cb6527be55ac56eecde0b731db2a97cbf180ac58..4dfd5999379411c74269a5c9f5c7c2bceb041871 100644
--- a/chrome/browser/win/jumplist.cc
+++ b/chrome/browser/win/jumplist.cc
@@ -250,46 +250,143 @@ JumpList::~JumpList() {
   Terminate();
 }
 
-// static
-bool JumpList::Enabled() {
-  return JumpListUpdater::IsEnabled();
+void JumpList::TopSitesLoaded(history::TopSites* top_sites) {}
+
+void JumpList::TopSitesChanged(history::TopSites* top_sites,
+                               ChangeReason change_reason) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
+  top_sites_has_pending_notification_ = true;
+
+  // Postpone handling this notification until a pending update completes.
+  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 JumpList in a while.
+  InitializeTimerForUpdate();
 }
 
-void JumpList::CancelPendingUpdate() {
+void JumpList::TabRestoreServiceChanged(sessions::TabRestoreService* service) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
-  // Cancel a pending most-visited URL fetch by invalidating the weak pointer.
-  weak_ptr_factory_.InvalidateWeakPtrs();
+  tab_restore_has_pending_notification_ = true;
 
-  // Cancel a pending favicon loading by invalidating its task id.
-  if (task_id_ != base::CancelableTaskTracker::kBadTaskId) {
-    cancelable_task_tracker_.TryCancel(task_id_);
-    task_id_ = base::CancelableTaskTracker::kBadTaskId;
+  // Postpone handling this notification until a pending update completes.
+  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 JumpList in a while.
+  InitializeTimerForUpdate();
+}
+
+void JumpList::TabRestoreServiceDestroyed(
+    sessions::TabRestoreService* service) {}
+
+void JumpList::InitializeTimerForUpdate() {
+  if (timer_.IsRunning()) {
+    timer_.Reset();
+  } else {
+    // base::Unretained is safe since |this| is guaranteed to outlive timer_.
+    timer_.Start(FROM_HERE, kDelayForJumplistUpdate,
+                 base::Bind(&JumpList::OnDelayTimer, base::Unretained(this)));
   }
 }
 
-void JumpList::Terminate() {
+void JumpList::OnDelayTimer() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  timer_.Stop();
-  CancelPendingUpdate();
-  update_in_progress_ = false;
-  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();
+
+  if (updates_to_skip_ > 0) {
+    --updates_to_skip_;
+    return;
   }
-  profile_ = nullptr;
+
+  // Retrieve the recently closed URLs synchronously.
+  if (tab_restore_has_pending_notification_) {
+    tab_restore_has_pending_notification_ = false;
+    ProcessTabRestoreServiceNotification();
+  }
+
+  // If TopSites has updates, retrieve the URLs asynchronously, and on its
+  // completion, trigger favicon loading.
+  // Otherwise, call StartLoadingFavicon directly to start favicon loading.
+  if (top_sites_has_pending_notification_)
+    ProcessTopSitesNotification();
+  else
+    StartLoadingFavicon();
 }
 
-void JumpList::Shutdown() {
+void JumpList::ProcessTopSitesNotification() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  Terminate();
+
+  // 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. All previous pending
+  // notifications from TopSites are ignored.
+  if (!has_tab_closed_) {
+    top_sites_has_pending_notification_ = false;
+    return;
+  }
+
+  scoped_refptr<history::TopSites> top_sites =
+      TopSitesFactory::GetForProfile(profile_);
+  if (top_sites) {
+    top_sites->GetMostVisitedURLs(
+        base::Bind(&JumpList::OnMostVisitedURLsAvailable,
+                   weak_ptr_factory_.GetWeakPtr()),
+        false);
+  }
+}
+
+void JumpList::ProcessTabRestoreServiceNotification() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
+  // 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().
+
+  sessions::TabRestoreService* tab_restore_service =
+      TabRestoreServiceFactory::GetForProfile(profile_);
+
+  recently_closed_pages_.clear();
+
+  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;
+    }
+  }
+
+  recently_closed_should_update_ = true;
+
+  // 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();
+  }
 }
 
 void JumpList::OnMostVisitedURLsAvailable(
@@ -326,25 +423,6 @@ void JumpList::OnMostVisitedURLsAvailable(
   StartLoadingFavicon();
 }
 
-void JumpList::TabRestoreServiceChanged(sessions::TabRestoreService* service) {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-
-  tab_restore_has_pending_notification_ = true;
-
-  // Postpone handling this notification until a pending update completes.
-  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 JumpList in a while.
-  InitializeTimerForUpdate();
-}
-
-void JumpList::TabRestoreServiceDestroyed(
-    sessions::TabRestoreService* service) {}
-
 bool JumpList::AddTab(const sessions::TabRestoreService::Tab& tab,
                       size_t max_items) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
@@ -485,198 +563,89 @@ void JumpList::PostRunUpdate() {
   }
 }
 
-void JumpList::TopSitesLoaded(history::TopSites* top_sites) {
-}
-
-void JumpList::TopSitesChanged(history::TopSites* top_sites,
-                               ChangeReason change_reason) {
+void JumpList::OnRunUpdateCompletion(
+    std::unique_ptr<UpdateResults> update_results) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
-  top_sites_has_pending_notification_ = true;
-
-  // Postpone handling this notification until a pending update completes.
-  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 JumpList in a while.
-  InitializeTimerForUpdate();
-}
+  // Update JumpList member variables based on the results from the update run
+  // just finished.
+  if (update_results->update_timeout)
+    updates_to_skip_ = kUpdatesToSkipUnderHeavyLoad;
 
-void JumpList::InitializeTimerForUpdate() {
-  if (timer_.IsRunning()) {
-    timer_.Reset();
-  } else {
-    // base::Unretained is safe since |this| is guaranteed to outlive timer_.
-    timer_.Start(FROM_HERE, kDelayForJumplistUpdate,
-                 base::Bind(&JumpList::OnDelayTimer, base::Unretained(this)));
+  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_should_update_ = false;
+    recently_closed_should_update_ = false;
   }
-}
 
-void JumpList::OnDelayTimer() {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  update_in_progress_ = false;
 
-  if (updates_to_skip_ > 0) {
-    --updates_to_skip_;
-    return;
-  }
+  // If there is any new notification during the update run just finished, start
+  // another JumpList update.
+  // Otherwise, post tasks to delete the JumpListIcons and JumpListIconsOld
+  // folders as they are no longer needed. Now we have the
+  // JumpListIcons{MostVisited, RecentClosed} folders instead.
+  if (top_sites_has_pending_notification_ ||
+      tab_restore_has_pending_notification_) {
+    InitializeTimerForUpdate();
+  } else {
+    base::FilePath profile_dir = profile_->GetPath();
+    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));
 
-  // Retrieve the recently closed URLs synchronously.
-  if (tab_restore_has_pending_notification_) {
-    tab_restore_has_pending_notification_ = false;
-    ProcessTabRestoreServiceNotification();
+    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));
   }
-
-  // If TopSites has updates, retrieve the URLs asynchronously, and on its
-  // completion, trigger favicon loading.
-  // Otherwise, call StartLoadingFavicon directly to start favicon loading.
-  if (top_sites_has_pending_notification_)
-    ProcessTopSitesNotification();
-  else
-    StartLoadingFavicon();
 }
 
-void JumpList::ProcessTopSitesNotification() {
+void JumpList::CancelPendingUpdate() {
   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. All previous pending
-  // notifications from TopSites are ignored.
-  if (!has_tab_closed_) {
-    top_sites_has_pending_notification_ = false;
-    return;
-  }
+  // Cancel a pending most-visited URL fetch by invalidating the weak pointer.
+  weak_ptr_factory_.InvalidateWeakPtrs();
 
-  scoped_refptr<history::TopSites> top_sites =
-      TopSitesFactory::GetForProfile(profile_);
-  if (top_sites) {
-    top_sites->GetMostVisitedURLs(
-        base::Bind(&JumpList::OnMostVisitedURLsAvailable,
-                   weak_ptr_factory_.GetWeakPtr()),
-        false);
+  // Cancel a pending favicon loading by invalidating its task id.
+  if (task_id_ != base::CancelableTaskTracker::kBadTaskId) {
+    cancelable_task_tracker_.TryCancel(task_id_);
+    task_id_ = base::CancelableTaskTracker::kBadTaskId;
   }
 }
 
-void JumpList::ProcessTabRestoreServiceNotification() {
+void JumpList::Terminate() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-
-  // 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().
-
-  sessions::TabRestoreService* tab_restore_service =
-      TabRestoreServiceFactory::GetForProfile(profile_);
-
-  recently_closed_pages_.clear();
-
-  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;
-    }
-  }
-
-  recently_closed_should_update_ = true;
-
-  // Force a TopSite history sync when closing a first tab in one session.
-  if (!has_tab_closed_) {
-    has_tab_closed_ = true;
+  timer_.Stop();
+  CancelPendingUpdate();
+  update_in_progress_ = false;
+  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->SyncWithHistory();
+      top_sites->RemoveObserver(this);
+    pref_change_registrar_.reset();
   }
+  profile_ = nullptr;
 }
 
-// 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(icon_cache->size());
-
-  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,
-                              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 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 = 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 {
-      base::FilePath icon_path;
-      if (CreateIconFile(item->icon_image(), icon_dir, &icon_path)) {
-        ++icons_created;
-        item->set_icon(icon_path.value(), 0);
-        updated_map[item->url()] = icon_path;
-      }
-    }
-  }
-  icon_cache->swap(updated_map);
-
-  return icons_created;
+void JumpList::Shutdown() {

[grt (UTC plus 2), 2017/06/14 10:54:12]

please put all methods here in the same order as those in the .h

[chengx, 2017/06/14 16:53:23]

Done.

+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  Terminate();
 }
 
 // static
-int JumpList::UpdateIconFiles(const base::FilePath& icon_dir,
-                              const ShellLinkItemList& page_list,
-                              size_t slot_limit,
-                              URLIconCache* icon_cache) {
-  int icons_created = 0;
-
-  // Clear the JumpList icon folder at |icon_dir| and the cache when
-  // 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);
-    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, icon_cache);
-  } else if (base::CreateDirectory(icon_dir)) {
-    icons_created +=
-        CreateIconFiles(icon_dir, page_list, slot_limit, icon_cache);
-    DeleteIconFiles(icon_dir, icon_cache);
-  }
-
-  return icons_created;
+bool JumpList::Enabled() {
+  return JumpListUpdater::IsEnabled();
 }
 
 // static
@@ -782,45 +751,75 @@ void JumpList::RunUpdateJumpList(
     update_results->update_timeout = true;
 }
 
-void JumpList::OnRunUpdateCompletion(
-    std::unique_ptr<UpdateResults> update_results) {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-
-  // Update JumpList member variables based on the results from the update run
-  // just finished.
-  if (update_results->update_timeout)
-    updates_to_skip_ = kUpdatesToSkipUnderHeavyLoad;
+// static
+int JumpList::UpdateIconFiles(const base::FilePath& icon_dir,
+                              const ShellLinkItemList& page_list,
+                              size_t slot_limit,
+                              URLIconCache* icon_cache) {
+  int icons_created = 0;
 
-  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_should_update_ = false;
-    recently_closed_should_update_ = false;
+  // Clear the JumpList icon folder at |icon_dir| and the cache when
+  // 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);
+    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, icon_cache);
+  } else if (base::CreateDirectory(icon_dir)) {
+    icons_created +=
+        CreateIconFiles(icon_dir, page_list, slot_limit, icon_cache);
+    DeleteIconFiles(icon_dir, icon_cache);
   }
 
-  update_in_progress_ = false;
+  return icons_created;
+}
 
-  // If there is any new notification during the update run just finished, start
-  // another JumpList update.
-  // Otherwise, post tasks to delete the JumpListIcons and JumpListIconsOld
-  // folders as they are no longer needed. Now we have the
-  // JumpListIcons{MostVisited, RecentClosed} folders instead.
-  if (top_sites_has_pending_notification_ ||
-      tab_restore_has_pending_notification_) {
-    InitializeTimerForUpdate();
-  } else {
-    base::FilePath profile_dir = profile_->GetPath();
-    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));
+// static
+int JumpList::CreateIconFiles(const base::FilePath& icon_dir,
+                              const ShellLinkItemList& item_list,
+                              size_t max_items,
+                              URLIconCache* icon_cache) {
+  // TODO(chengx): Remove the UMA histogram after fixing http://crbug.com/40407.
+  SCOPED_UMA_HISTOGRAM_TIMER("WinJumplist.CreateIconFilesDuration");
 
-    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));
+  int icons_created = 0;
+
+  // 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 = 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 {
+      base::FilePath icon_path;
+      if (CreateIconFile(item->icon_image(), icon_dir, &icon_path)) {
+        ++icons_created;
+        item->set_icon(icon_path.value(), 0);
+        updated_map[item->url()] = icon_path;
+      }
+    }
   }
+  icon_cache->swap(updated_map);
+
+  return icons_created;
+}
+
+// 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(icon_cache->size());
+
+  for (const auto& url_path_pair : *icon_cache)
+    cached_files.insert(url_path_pair.second);
+
+  DeleteNonCachedFiles(icon_dir, cached_files);
 }