Delete the right JumpList icons conditional on shell notification

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 bbca037062d132ddc5c693646170827aee422cee..3eeda2e95f8eac9dd8feb2edefc6b6cc2cae57a9 100644
--- a/chrome/browser/win/jumplist.cc
+++ b/chrome/browser/win/jumplist.cc
@@ -194,9 +194,9 @@ base::FilePath GenerateJumplistIconDirName(
 
 }  // namespace
 
-JumpList::UpdateResults::UpdateResults() {}
+JumpList::UpdateTransaction::UpdateTransaction() {}
 
-JumpList::UpdateResults::~UpdateResults() {}
+JumpList::UpdateTransaction::~UpdateTransaction() {}
 
 // static
 bool JumpList::Enabled() {
@@ -552,35 +552,38 @@ void JumpList::PostRunUpdate() {
   // 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_should_update = most_visited_should_update_;
   bool recently_closed_should_update = recently_closed_should_update_;
 
-  auto update_results = base::MakeUnique<UpdateResults>();
-  update_results->most_visited_icons_in_update = most_visited_icons_;
-  update_results->recently_closed_icons_in_update = recently_closed_icons_;
+  auto update_results = base::MakeUnique<UpdateTransaction>();
+  update_results->most_visited_icons = most_visited_icons_;
+  update_results->recently_closed_icons = recently_closed_icons_;
 
-  // Parameter evaluation order is unspecified in C++. Ensure the pointer value
-  // is obtained before base::Passed() is called.
-  auto* update_results_raw = update_results.get();
+  // Parameter evaluation order is unspecified in C++. Do the first bind and
+  // then move it into PostTaskAndReply to ensure the pointer value is obtained
+  // before base::Passed() is called.
+  auto run_update =
+      base::Bind(&JumpList::RunUpdateJumpList, app_id_, profile_dir,
+                 local_most_visited_pages, local_recently_closed_pages,
+                 most_visited_should_update, recently_closed_should_update,

[grt (UTC plus 2), 2017/06/23 09:36:14]

is it possible for either this->*_should_update_ to change state while the
update is running? i thought that with the latest design, update_in_progress_
causes the handling of inbound notifications to be deferred until the running
update completes. if this is correct, there's no need to bind these two bools
into the callback -- they can be read from |this| in OnRunUpdateCompletion.

perhaps add a DCHECK(!update_in_progress_) to OnDelayTimer,
ProcessTabRestoreServiceNotification, and OnMostVisitedURLsAvailable to make
this even more clear.

[chengx, 2017/06/23 18:52:38]

You are right. There is no need to bind these two bools into the callback for
OnRunUpdateCompletion. I've also added the DCHECK in various places.

+                 incognito_availability, update_results.get());
 
   // Post a task to update the JumpList, which consists of 1) create new icons,
-  // 2) delete old icons, 3) notify the OS.
+  // 2) notify the OS, 3) delete old icons.
   if (!update_jumplist_task_runner_->PostTaskAndReply(
-          FROM_HERE,
-          base::Bind(&JumpList::RunUpdateJumpList, app_id_, profile_dir,
-                     local_most_visited_pages, local_recently_closed_pages,
-                     most_visited_should_update, recently_closed_should_update,
-                     incognito_availability, update_results_raw),
-          base::Bind(&JumpList::OnRunUpdateCompletion,
-                     weak_ptr_factory_.GetWeakPtr(),
-                     base::Passed(std::move(update_results))))) {
-    OnRunUpdateCompletion(base::MakeUnique<UpdateResults>());
+          FROM_HERE, std::move(run_update),
+          base::Bind(
+              &JumpList::OnRunUpdateCompletion, weak_ptr_factory_.GetWeakPtr(),
+              base::Passed(std::move(update_results)),
+              most_visited_should_update, recently_closed_should_update))) {
+    OnRunUpdateCompletion(base::MakeUnique<UpdateTransaction>(), false, false);
   }
 }
 
 void JumpList::OnRunUpdateCompletion(
-    std::unique_ptr<UpdateResults> update_results) {
+    std::unique_ptr<UpdateTransaction> update_results,
+    bool most_visited_should_update,
+    bool recently_closed_should_update) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
   // Update JumpList member variables based on the results from the update run
@@ -589,11 +592,14 @@ void JumpList::OnRunUpdateCompletion(
     updates_to_skip_ = kUpdatesToSkipUnderHeavyLoad;
 
   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;
+    if (most_visited_should_update) {
+      most_visited_icons_.swap(update_results->most_visited_icons);

[grt (UTC plus 2), 2017/06/23 09:36:14]

nit: swap() used to be the efficient way to move stuff from |here| to |there|
when you didn't need to access |here| any longer. now that we have move
assignment operators, we have the same efficiency with:
      most_visited_icons_ = std::move(update_results->most_visited_icons);
the only difference really being the point at which the old contents of |there|
are destroyed. what do you think of replacing all occurrences of swap() in here
with move assignment? i think it more clearly expresses the intent than swap().

[chengx, 2017/06/23 18:52:38]

Done. I think std::move better conveys the intent here as what we need here is
merely an efficient assignment, rather than exchanging content.

+      most_visited_should_update_ = false;
+    }
+    if (recently_closed_should_update) {
+      recently_closed_icons_.swap(update_results->recently_closed_icons);
+      recently_closed_should_update_ = false;
+    }
   }
 
   update_in_progress_ = false;
@@ -655,18 +661,16 @@ void JumpList::Terminate() {
 }
 
 // static
-void JumpList::RunUpdateJumpList(
+void JumpList::CreateNewJumpListAndNotifyOS(
     const base::string16& app_id,
-    const base::FilePath& profile_dir,
+    const base::FilePath& most_visited_icon_dir,
+    const base::FilePath& recently_closed_icon_dir,
     const ShellLinkItemList& most_visited_pages,
     const ShellLinkItemList& recently_closed_pages,
     bool most_visited_should_update,
     bool recently_closed_should_update,
     IncognitoModePrefs::Availability incognito_availability,
-    UpdateResults* update_results) {
-  if (!JumpListUpdater::IsEnabled())
-    return;
-
+    UpdateTransaction* update_results) {
   DCHECK(update_results);
 
   JumpListUpdater jumplist_updater(app_id);
@@ -678,8 +682,7 @@ void JumpList::RunUpdateJumpList(
 
   // 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.
+  // steps will also take a long time.
   if (begin_update_timer.Elapsed() >= kTimeOutForJumplistBeginUpdate) {
     update_results->update_timeout = true;
     return;
@@ -688,24 +691,21 @@ void JumpList::RunUpdateJumpList(
   // Record the desired number of icons created in this JumpList update.
   int icons_created = 0;
 
+  URLIconCache most_visited_icons_next;
+  URLIconCache recently_closed_icons_next;
+
   // Update the icons for "Most Visisted" category of the JumpList if needed.
   if (most_visited_should_update) {
-    base::FilePath icon_dir_most_visited = GenerateJumplistIconDirName(
-        profile_dir, FILE_PATH_LITERAL("MostVisited"));
-
     icons_created += UpdateIconFiles(
-        icon_dir_most_visited, most_visited_pages, kMostVisitedItems,
-        &update_results->most_visited_icons_in_update);
+        most_visited_icon_dir, most_visited_pages, kMostVisitedItems,
+        &update_results->most_visited_icons, &most_visited_icons_next);
   }
 
   // Update the icons for "Recently Closed" category of the JumpList if needed.
   if (recently_closed_should_update) {
-    base::FilePath icon_dir_recent_closed = GenerateJumplistIconDirName(
-        profile_dir, FILE_PATH_LITERAL("RecentClosed"));
-
     icons_created += UpdateIconFiles(
-        icon_dir_recent_closed, recently_closed_pages, kRecentlyClosedItems,
-        &update_results->recently_closed_icons_in_update);
+        recently_closed_icon_dir, recently_closed_pages, kRecentlyClosedItems,
+        &update_results->recently_closed_icons, &recently_closed_icons_next);
   }
 
   // TODO(chengx): Remove the UMA histogram after fixing http://crbug.com/40407.
@@ -732,16 +732,13 @@ void JumpList::RunUpdateJumpList(
     return;
   }
 
-  // If JumpListUpdater::AddCustomCategory or JumpListUpdater::CommitUpdate
-  // takes longer than the maximum allowed time, skip the next
-  // |kUpdatesToSkipUnderHeavyLoad| updates. This update should be finished
-  // because we've already updated the icons on the disk. If discarding this
-  // update from here, some items in the current JumpList may not have icons
-  // as they've been delete from the disk. In this case, the background color of
-  // the JumpList panel is used instead, which doesn't look nice.
-
-  if (add_custom_category_timer.Elapsed() >= kTimeOutForAddCustomCategory)
+  // If AddCustomCategory call takes longer than the maximum allowed time, abort
+  // the current update and skip the next |kUpdatesToSkipUnderHeavyLoad|
+  // updates.
+  if (add_custom_category_timer.Elapsed() >= kTimeOutForAddCustomCategory) {
     update_results->update_timeout = true;
+    return;
+  }
 
   // Update the "Tasks" category of the JumpList.
   if (!UpdateTaskCategory(&jumplist_updater, incognito_availability))
@@ -750,35 +747,85 @@ void JumpList::RunUpdateJumpList(
   base::ElapsedTimer commit_update_timer;
 
   // Commit this transaction and send the updated JumpList to Windows.
-  if (jumplist_updater.CommitUpdate())
-    update_results->update_success = true;
+  bool commit_success = jumplist_updater.CommitUpdate();
 
+  // If CommitUpdate call takes longer than the maximum allowed time, abort the
+  // current update and skip the next |kUpdatesToSkipUnderHeavyLoad| updates.

[grt (UTC plus 2), 2017/06/23 09:36:14]

this doesn't abort the current update, as the current update is complete at this
point -- it just skips the next N updates.

[chengx, 2017/06/23 18:52:38]

Done.

   if (commit_update_timer.Elapsed() >= kTimeOutForJumplistCommitUpdate)
     update_results->update_timeout = true;
+
+  if (commit_success) {
+    update_results->update_success = true;
+    // The swaps below ensure update_results always has the icons to keep.
+    if (most_visited_should_update)
+      update_results->most_visited_icons.swap(most_visited_icons_next);
+    if (recently_closed_should_update)
+      update_results->recently_closed_icons.swap(recently_closed_icons_next);
+  }
+}
+
+// static
+void JumpList::RunUpdateJumpList(

[grt (UTC plus 2), 2017/06/23 09:36:14]

nit: move this above CreateNewJumpListAndNotifyOS?

[chengx, 2017/06/23 18:52:38]

Done.

+    const base::string16& app_id,
+    const base::FilePath& profile_dir,
+    const ShellLinkItemList& most_visited_pages,
+    const ShellLinkItemList& recently_closed_pages,
+    bool most_visited_should_update,
+    bool recently_closed_should_update,
+    IncognitoModePrefs::Availability incognito_availability,
+    UpdateTransaction* update_results) {
+  if (!JumpListUpdater::IsEnabled())

[grt (UTC plus 2), 2017/06/23 09:36:14]

is this needed? can IsEnabled() change from true to false within a given
process? in other words, is it possible for RunUpdateJumpList to be called if
IsEnabled() was false? this seems awfully late in the game to check for
enablement if the state can't flip during execution.

[chengx, 2017/06/23 18:52:38]

I think it should be moved to the very beginning of the jumplist code. I prefer
to use another small CL for this though.

+    return;
+
+  DCHECK(update_results);
+
+  base::FilePath most_visited_icon_dir = GenerateJumplistIconDirName(
+      profile_dir, FILE_PATH_LITERAL("MostVisited"));
+  base::FilePath recently_closed_icon_dir = GenerateJumplistIconDirName(
+      profile_dir, FILE_PATH_LITERAL("RecentClosed"));
+
+  CreateNewJumpListAndNotifyOS(
+      app_id, most_visited_icon_dir, recently_closed_icon_dir,
+      most_visited_pages, recently_closed_pages, most_visited_should_update,
+      recently_closed_should_update, incognito_availability, update_results);
+
+  // Delete any obsolete icon files.
+  if (most_visited_should_update)
+    DeleteIconFiles(most_visited_icon_dir, update_results->most_visited_icons);
+  if (recently_closed_should_update) {
+    DeleteIconFiles(recently_closed_icon_dir,
+                    update_results->recently_closed_icons);
+  }
 }
 
 // static
 int JumpList::UpdateIconFiles(const base::FilePath& icon_dir,
                               const ShellLinkItemList& page_list,
                               size_t slot_limit,
-                              URLIconCache* icon_cache) {
+                              URLIconCache* icon_cur,
+                              URLIconCache* icon_next) {
+  DCHECK(icon_cur);
+  DCHECK(icon_next);
+
   int icons_created = 0;

[grt (UTC plus 2), 2017/06/23 09:36:14]

nit: how about cleaning this up a tiny bit:

  // Clear the JumpList icon folder at |icon_dir| and the cache when
  // 1) |icon_cur| 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_cur->empty() || FilesExceedLimitInDir(icon_dir, slot_limit * 2)) {
    DeleteDirectoryContentAndLogRuntime(icon_dir, kFileDeleteLimit);
    icon_cur->clear();
    icon_next->clear();
    // Create new icons only when the directory exists and is empty.
    if (!base::CreateDirectory(icon_dir) || !base::IsDirectoryEmpty(icon_dir))
      return 0;
  } else if (!base::CreateDirectory(icon_dir)) {
    return 0;
  }

  return CreateIconFiles(icon_dir, page_list, slot_limit, icon_cur, icon_next);

[chengx, 2017/06/23 18:52:38]

Done. Thanks for the suggestion!

 
-  // Clear the JumpList icon folder at |icon_dir| and the cache when
-  // 1) |icon_cache| is empty. This happens when "Most visited" or "Recently
+  // Clear the JumpList icon folder at |icon_dir| and the caches when
+  // 1) |icon_cur| 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)) {
+  if (icon_cur->empty() || FilesExceedLimitInDir(icon_dir, slot_limit * 2)) {
     DeleteDirectoryContentAndLogRuntime(icon_dir, kFileDeleteLimit);
-    icon_cache->clear();
+    icon_cur->clear();
+    icon_next->clear();
+
     // Create new icons only when the directory exists and is empty.
-    if (base::CreateDirectory(icon_dir) && base::IsDirectoryEmpty(icon_dir))
+    if (base::CreateDirectory(icon_dir) && base::IsDirectoryEmpty(icon_dir)) {
       icons_created +=
-          CreateIconFiles(icon_dir, page_list, slot_limit, icon_cache);
+          CreateIconFiles(icon_dir, page_list, slot_limit, icon_cur, icon_next);
+    }
   } else if (base::CreateDirectory(icon_dir)) {
     icons_created +=
-        CreateIconFiles(icon_dir, page_list, slot_limit, icon_cache);
-    DeleteIconFiles(icon_dir, icon_cache);
+        CreateIconFiles(icon_dir, page_list, slot_limit, icon_cur, icon_next);
   }
 
   return icons_created;
@@ -788,43 +835,45 @@ int JumpList::UpdateIconFiles(const base::FilePath& icon_dir,
 int JumpList::CreateIconFiles(const base::FilePath& icon_dir,
                               const ShellLinkItemList& item_list,
                               size_t max_items,
-                              URLIconCache* icon_cache) {
+                              URLIconCache* icon_cur,

[grt (UTC plus 2), 2017/06/23 09:36:14]

nit: pass |icon_cur| by constref

[chengx, 2017/06/23 18:52:38]

Done.

+                              URLIconCache* icon_next) {
+  DCHECK(icon_cur);
+  DCHECK(icon_next);
+
   // 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()) {
+    auto cache_iter = icon_cur->find(item->url());
+    if (cache_iter != icon_cur->end()) {
       item->set_icon(cache_iter->second.value(), 0);
-      updated_map[item->url()] = cache_iter->second;
+      (*icon_next)[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_next)[item->url()] = icon_path;
       }
     }
   }
-  icon_cache->swap(updated_map);
 
   return icons_created;
 }
 
 // static
 void JumpList::DeleteIconFiles(const base::FilePath& icon_dir,
-                               URLIconCache* icon_cache) {
+                               const URLIconCache& icons_cache) {
   // Put all cached icon file paths into a set.
   base::flat_set<base::FilePath> cached_files;
-  cached_files.reserve(icon_cache->size());
+  cached_files.reserve(icons_cache.size());
 
-  for (const auto& url_path_pair : *icon_cache)
+  for (const auto& url_path_pair : icons_cache)
     cached_files.insert(url_path_pair.second);
 
   DeleteNonCachedFiles(icon_dir, cached_files);