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..e387136c10ae321def80fe9b37f58b067fe2c178 100644
--- a/chrome/browser/win/jumplist.cc
+++ b/chrome/browser/win/jumplist.cc
@@ -73,16 +73,16 @@ constexpr base::TimeDelta kDelayForJumplistUpdate =
 
 // The maximum allowed time for JumpListUpdater::BeginUpdate. Updates taking
 // longer than this are discarded to prevent bogging down slow machines.
-constexpr base::TimeDelta kTimeOutForJumplistBeginUpdate =
+constexpr base::TimeDelta kTimeOutForBeginUpdate =
     base::TimeDelta::FromMilliseconds(500);
 
 // The maximum allowed time for updating both "most visited" and "recently
 // closed" categories via JumpListUpdater::AddCustomCategory.
-constexpr base::TimeDelta kTimeOutForAddCustomCategory =
+constexpr base::TimeDelta kTimeOutForAddCategory =
     base::TimeDelta::FromMilliseconds(500);
 
 // The maximum allowed time for JumpListUpdater::CommitUpdate.
-constexpr base::TimeDelta kTimeOutForJumplistCommitUpdate =
+constexpr base::TimeDelta kTimeOutForCommitUpdate =
     base::TimeDelta::FromMilliseconds(1000);
 
 // Appends the common switches to each shell link.
@@ -552,35 +552,39 @@ 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_;
+  update_results->most_visited_icons = most_visited_icons_;
+  update_results->recently_closed_icons = recently_closed_icons_;
+  update_results->most_visited_icon_assoc = most_visited_icon_assoc_;
+  update_results->recently_closed_icon_assoc = recently_closed_icon_assoc_;
 
   // 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();

[grt (UTC plus 2), 2017/06/21 10:57:29]

wdyt of moving the first Bind out here now so that you don't have a naked
pointer?

[chengx, 2017/06/22 22:45:06]

Done.

 
   // 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>());
+          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<UpdateResults>(), false, false);
   }
 }
 
 void JumpList::OnRunUpdateCompletion(
-    std::unique_ptr<UpdateResults> update_results) {
+    std::unique_ptr<UpdateResults> 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 +593,17 @@ 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);
+      most_visited_icon_assoc_.swap(update_results->most_visited_icon_assoc);
+      most_visited_should_update_ = false;
+    }
+    if (recently_closed_should_update) {
+      recently_closed_icons_.swap(update_results->recently_closed_icons);
+      recently_closed_icon_assoc_.swap(
+          update_results->recently_closed_icon_assoc);
+      recently_closed_should_update_ = false;
+    }
   }
 
   update_in_progress_ = false;
@@ -680,7 +690,7 @@ void JumpList::RunUpdateJumpList(
   // 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) {
+  if (begin_update_timer.Elapsed() >= kTimeOutForBeginUpdate) {
     update_results->update_timeout = true;
     return;
   }
@@ -688,24 +698,28 @@ 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;
+
+  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"));
+
   // 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);
+    icons_created += SafeCreateIconFiles(

[grt (UTC plus 2), 2017/06/21 10:57:29]

if i understand correctly, here and on line 719 you want to create the new icon
files that, if all goes well, will become *the* icon files for the jumplist. any
failure between here and the end of this function should result in the new files
being deleted. rather than having repeated "delete the new files" code blocks at
the early exit points (lines 751, 760, and 789 in case of error), one way to
slice this is to introduce the idea of a "transaction" that is in a pending
state until it is committed. i'm imagining that an IconCache is its own object
with a few methods, one of which is BeginTransaction, and returns some object
that is held on the stack. anything stack unwind will lead to its destruction,
which will roll-back any icon creation that took place. this transaction object
has an additional Commit method that, when called, causes the new icons to
persist and deletes the old icons that are no longer relevant. this will make
the code here considerably cleaner.

i suggest that you spend some time thinking about what some simple IconCache and
IconCache::Transaction classes might look like. i think this would lend itself
to some nice encapsulation and would be easily unit-tested.

[chengx, 2017/06/22 22:45:06]

Thanks a lot for the suggestion and your understanding to my code is absolutely
correct. I agree the transaction concept fits well here. But to be honest, I
don't think it requires so much modification as what you suggested. I have
created a wrapper function called CreateNewJumpListAndNotifyOS to create a new
JumpList and notify OS. The icon deletion is separated from this new API, so
that there is only place for icon deletion now. 

+        most_visited_icon_dir, most_visited_pages, kMostVisitedItems,
+        &update_results->most_visited_icon_assoc,
+        &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);
+    icons_created += SafeCreateIconFiles(
+        recently_closed_icon_dir, recently_closed_pages, kRecentlyClosedItems,
+        &update_results->recently_closed_icon_assoc,
+        &update_results->recently_closed_icons, &recently_closed_icons_next);
   }
 
   // TODO(chengx): Remove the UMA histogram after fixing http://crbug.com/40407.
@@ -716,69 +730,98 @@ void JumpList::RunUpdateJumpList(
 
   base::ElapsedTimer add_custom_category_timer;
 
-  // Update the "Most Visited" category of the JumpList if it exists.
-  // This update request is applied into the JumpList when we commit this
-  // transaction.
-  if (!jumplist_updater.AddCustomCategory(
+  // Update the JumpList categories. This update request is applied into the
+  // JumpList when we commit this transaction.
+  bool add_category_success =
+      jumplist_updater.AddCustomCategory(
           l10n_util::GetStringUTF16(IDS_NEW_TAB_MOST_VISITED),
-          most_visited_pages, kMostVisitedItems)) {
-    return;
-  }
-
-  // Update the "Recently Closed" category of the JumpList.
-  if (!jumplist_updater.AddCustomCategory(
+          most_visited_pages, kMostVisitedItems) &&
+      jumplist_updater.AddCustomCategory(
           l10n_util::GetStringUTF16(IDS_RECENTLY_CLOSED), recently_closed_pages,
-          kRecentlyClosedItems)) {
-    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.
+          kRecentlyClosedItems);
 
-  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.
+  bool timeout = add_custom_category_timer.Elapsed() >= kTimeOutForAddCategory;
+  if (timeout)
     update_results->update_timeout = true;
 
+  if (!add_category_success || timeout) {
+    DeleteIconFilesUnified(most_visited_should_update,
+                           recently_closed_should_update, most_visited_icon_dir,
+                           recently_closed_icon_dir, update_results,
+                           JumpListVersion::kNext);
+    return;
+  }
+
   // Update the "Tasks" category of the JumpList.
-  if (!UpdateTaskCategory(&jumplist_updater, incognito_availability))
+  if (!UpdateTaskCategory(&jumplist_updater, incognito_availability)) {
+    DeleteIconFilesUnified(most_visited_should_update,
+                           recently_closed_should_update, most_visited_icon_dir,
+                           recently_closed_icon_dir, update_results,
+                           JumpListVersion::kNext);
     return;
+  }
 
   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 (commit_update_timer.Elapsed() >= kTimeOutForJumplistCommitUpdate)
+  // If CommitUpdate call takes longer than the maximum allowed time, abort the
+  // current update and skip the next |kUpdatesToSkipUnderHeavyLoad| updates.
+  if (commit_update_timer.Elapsed() >= kTimeOutForCommitUpdate)
     update_results->update_timeout = true;
+
+  if (commit_success) {
+    update_results->update_success = true;
+    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);
+  }
+
+  // Delete the set of icons based on commit status.
+  JumpListVersion version =
+      commit_success ? JumpListVersion::kCurrent : JumpListVersion::kNext;
+
+  DeleteIconFilesUnified(most_visited_should_update,
+                         recently_closed_should_update, most_visited_icon_dir,
+                         recently_closed_icon_dir, update_results, version);
 }
 
 // static
-int JumpList::UpdateIconFiles(const base::FilePath& icon_dir,
-                              const ShellLinkItemList& page_list,
-                              size_t slot_limit,
-                              URLIconCache* icon_cache) {
+int JumpList::SafeCreateIconFiles(const base::FilePath& icon_dir,
+                                  const ShellLinkItemList& page_list,
+                                  size_t slot_limit,
+                                  IconAssociation* icon_assoc,
+                                  URLIconCache* icon_cur,
+                                  URLIconCache* icon_next) {
+  DCHECK(icon_assoc);
+  DCHECK(icon_cur);
+  DCHECK(icon_next);
+
   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
+  // 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_assoc->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))
-      icons_created +=
-          CreateIconFiles(icon_dir, page_list, slot_limit, icon_cache);
+    if (base::CreateDirectory(icon_dir) && base::IsDirectoryEmpty(icon_dir)) {
+      icons_created += CreateIconFiles(icon_dir, page_list, slot_limit,
+                                       icon_assoc, 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);
+    icons_created += CreateIconFiles(icon_dir, page_list, slot_limit,
+                                     icon_assoc, icon_cur, icon_next);
   }
 
   return icons_created;
@@ -788,44 +831,85 @@ 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) {
+                              IconAssociation* icon_assoc,
+                              URLIconCache* icon_cur,
+                              URLIconCache* icon_next) {
+  DCHECK(icon_assoc);
+  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;
 
+  // Mark all the icons kCurrent as they are used by the current JumpList.
+  for (auto iter = icon_assoc->begin(); iter != icon_assoc->end(); ++iter)

[grt (UTC plus 2), 2017/06/21 10:57:29]

  std::for_each(std::begin(*icon_assoc), std::end(*icon_assoc),
                [] (auto &value) { value.second = JumpListVersion::kCurrent; });

or

  for (auto& value : *icon_assoc)
    value.second = JumpListVersion::kCurrent;

[chengx, 2017/06/22 22:45:06]

Done. I'll go with the second method.

+    iter->second = JumpListVersion::kCurrent;
+
   // 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;
+      (*icon_assoc)[cache_iter->second] = JumpListVersion::kShared;
     } 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_assoc)[icon_path] = JumpListVersion::kNext;
       }
     }
   }
-  icon_cache->swap(updated_map);
 
   return icons_created;
 }
 
 // static
+void JumpList::DeleteIconFilesUnified(
+    bool most_visited_should_update,
+    bool recently_closed_should_update,
+    const base::FilePath& most_visited_icon_dir,
+    const base::FilePath& recently_closed_icon_dir,
+    UpdateResults* update_results,
+    JumpListVersion version) {
+  DCHECK(update_results);
+
+  if (most_visited_should_update) {
+    DeleteIconFiles(most_visited_icon_dir,
+                    &update_results->most_visited_icon_assoc, version);
+  }
+  if (recently_closed_should_update) {
+    DeleteIconFiles(recently_closed_icon_dir,
+                    &update_results->recently_closed_icon_assoc, version);
+  }
+}
+
+// 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());
+                               IconAssociation* icon_assoc,
+                               JumpListVersion version) {
+  DCHECK(icon_assoc);
 
-  for (const auto& url_path_pair : *icon_cache)
-    cached_files.insert(url_path_pair.second);
+  IconAssociation icon_assoc_updated;
+
+  // Put paths of icon files to keep into a set.
+  base::flat_set<base::FilePath> cached_files;
+  cached_files.reserve(icon_assoc->size());
+  for (const auto& path_version_pair : *icon_assoc) {
+    if (path_version_pair.second != version) {
+      cached_files.insert(path_version_pair.first);
+      icon_assoc_updated[path_version_pair.first] = path_version_pair.second;
+    }
+  }
 
   DeleteNonCachedFiles(icon_dir, cached_files);
+
+  // Keep icon_assoc up-to-date after deleting obsolete icons.
+  icon_assoc->swap(icon_assoc_updated);
 }