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..d996f52841ca894a7da6e57f5acace48e904484d 100644
--- a/chrome/browser/win/jumplist.cc
+++ b/chrome/browser/win/jumplist.cc
@@ -61,7 +61,7 @@ namespace {
 // For the remaining 8 slots, we allocate 5 slots to "most-visited" items and 3
 // slots to "recently-closed" items, respectively.
 constexpr size_t kMostVisitedItems = 5;
-constexpr size_t kRecentlyClosedItems = 3;
+constexpr size_t kRecentClosedItems = 3;
 
 // The number of updates to skip to alleviate the machine when a previous update
 // was too slow.
@@ -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.
@@ -376,25 +376,25 @@ void JumpList::ProcessTabRestoreServiceNotification() {
   sessions::TabRestoreService* tab_restore_service =
       TabRestoreServiceFactory::GetForProfile(profile_);
 
-  recently_closed_pages_.clear();
+  recent_closed_pages_.clear();
 
   for (const auto& entry : tab_restore_service->entries()) {
-    if (recently_closed_pages_.size() >= kRecentlyClosedItems)
+    if (recent_closed_pages_.size() >= kRecentClosedItems)
       break;
     switch (entry->type) {
       case sessions::TabRestoreService::TAB:
         AddTab(static_cast<const sessions::TabRestoreService::Tab&>(*entry),
-               kRecentlyClosedItems);
+               kRecentClosedItems);
         break;
       case sessions::TabRestoreService::WINDOW:
         AddWindow(
             static_cast<const sessions::TabRestoreService::Window&>(*entry),
-            kRecentlyClosedItems);
+            kRecentClosedItems);
         break;
     }
   }
 
-  recently_closed_should_update_ = true;
+  recent_closed_should_update_ = true;
 
   // Force a TopSite history sync when closing a first tab in one session.
   if (!has_tab_closed_) {
@@ -447,7 +447,7 @@ bool JumpList::AddTab(const sessions::TabRestoreService::Tab& tab,
 
   // This code adds the URL and the title strings of the given tab to the
   // JumpList variables.
-  if (recently_closed_pages_.size() >= max_items)
+  if (recent_closed_pages_.size() >= max_items)
     return false;
 
   scoped_refptr<ShellLinkItem> link = CreateShellLink();
@@ -459,8 +459,8 @@ bool JumpList::AddTab(const sessions::TabRestoreService::Tab& tab,
                                             jumplist::kRecentlyClosedCategory);
   link->set_title(current_navigation.title());
   link->set_url(url);
-  recently_closed_pages_.push_back(link);
-  if (recently_closed_icons_.find(url) == recently_closed_icons_.end())
+  recent_closed_pages_.push_back(link);
+  if (recent_closed_icons_.find(url) == recent_closed_icons_.end())
     icon_urls_.emplace_back(std::move(url), std::move(link));
 
   return true;
@@ -551,26 +551,26 @@ 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_;
+  ShellLinkItemList local_recent_closed_pages = recent_closed_pages_;
 
   auto update_results = base::MakeUnique<UpdateResults>();

[grt (UTC plus 2), 2017/06/20 10:29:18]

meta comment: this object does more than just carry results back to the JumpList
instance. what differentiates the two lists above from the data being put into
|update_results|? perhaps this should become UpdateTransaction if it has inputs
and outputs.

also, does it make sense for JumpList to create an instance of this struct in
its ctor and have that one instance get bounced between the JL and the updating
thread rather than creating it anew for each update?

[chengx, 2017/06/20 19:33:59]

The two lists above, i.e., *_pages_, are not updated by the updating thread, so
they are merely inputs. I've also removed *_should_update from |update_results|
as they are also read-only in the updating thread. I added them there there
because I wanted to reduce the number of params passed by a few methods.
However, this seems to be confusing.

The data being put into |update_results| now include *_icons and *_icons_assoc.
They are all updated by the updating thread, which means they are indeed "update
results". So I will keep the struct's name as UpdateResults.
I feel it reads better with the current impl though.

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

So they're in-out params, yes? I can see how they are results in the sense that
they are the things taken away from the update and applied back to the JumpList
instance, but they're not strictly results. Reducing the number of values that
are bound in the callback is worthwhile. I think it's still worth considering
calling it an UpdateTransaction and using doc comments and layout to make it
clear which fields are in params, which are in-out params, and which (if any)
are out params.
I'm not sure that there's a big difference. There may be less code and fewer
copies of data being made if ownership of one instance of some struct bounces
back and forth. Maybe it wouldn't make sense for all fields to be within this
struct though.

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

Yes, they're in-out params. I've renamed the enum class to UpdateTransaction and
added the comments.
IMHO, I don't think it makes a big difference given that there are only 4 fields
(including 2 bools) in this struct now. 

-  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->recent_closed_icons = recent_closed_icons_;
+  update_results->most_visited_icon_assoc = most_visited_icon_assoc_;
+  update_results->recent_closed_icon_assoc = recent_closed_icon_assoc_;
+  update_results->most_visited_should_update = most_visited_should_update_;
+  update_results->recent_closed_should_update = recent_closed_should_update_;
 
   // 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();
 
   // 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,
+                     local_most_visited_pages, local_recent_closed_pages,
                      incognito_availability, update_results_raw),
           base::Bind(&JumpList::OnRunUpdateCompletion,
                      weak_ptr_factory_.GetWeakPtr(),
@@ -589,11 +589,16 @@ 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 (update_results->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 (update_results->recent_closed_should_update) {
+      recent_closed_icons_.swap(update_results->recent_closed_icons);
+      recent_closed_icon_assoc_.swap(update_results->recent_closed_icon_assoc);
+      recent_closed_should_update_ = false;
+    }
   }
 
   update_in_progress_ = false;
@@ -659,9 +664,7 @@ 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_should_update,
-    bool recently_closed_should_update,
+    const ShellLinkItemList& recent_closed_pages,
     IncognitoModePrefs::Availability incognito_availability,
     UpdateResults* update_results) {
   if (!JumpListUpdater::IsEnabled())
@@ -680,7 +683,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 +691,28 @@ void JumpList::RunUpdateJumpList(
   // Record the desired number of icons created in this JumpList update.
   int icons_created = 0;
 
-  // 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"));
+  URLIconCache most_visited_icons_next;
+  URLIconCache recent_closed_icons_next;
+
+  base::FilePath most_visited_icon_dir = GenerateJumplistIconDirName(
+      profile_dir, FILE_PATH_LITERAL("MostVisited"));
+  base::FilePath recent_closed_icon_dir = GenerateJumplistIconDirName(
+      profile_dir, FILE_PATH_LITERAL("RecentClosed"));
 
-    icons_created += UpdateIconFiles(
-        icon_dir_most_visited, most_visited_pages, kMostVisitedItems,
-        &update_results->most_visited_icons_in_update);
+  // Update the icons for "Most Visisted" category of the JumpList if needed.
+  if (update_results->most_visited_should_update) {
+    icons_created += SafeCreateIconFiles(
+        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);
+  if (update_results->recent_closed_should_update) {
+    icons_created += SafeCreateIconFiles(
+        recent_closed_icon_dir, recent_closed_pages, kRecentClosedItems,
+        &update_results->recent_closed_icon_assoc,
+        &update_results->recent_closed_icons, &recent_closed_icons_next);
   }
 
   // TODO(chengx): Remove the UMA histogram after fixing http://crbug.com/40407.
@@ -716,69 +723,93 @@ 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;
-  }
+          most_visited_pages, kMostVisitedItems) &&
+      jumplist_updater.AddCustomCategory(
+          l10n_util::GetStringUTF16(IDS_RECENTLY_CLOSED), recent_closed_pages,
+          kRecentClosedItems);
+
+  // 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;
 
-  // Update the "Recently Closed" category of the JumpList.
-  if (!jumplist_updater.AddCustomCategory(
-          l10n_util::GetStringUTF16(IDS_RECENTLY_CLOSED), recently_closed_pages,
-          kRecentlyClosedItems)) {
+  if (!add_category_success || timeout) {
+    DeleteIconFilesUnified(most_visited_icon_dir, recent_closed_icon_dir,
+                           update_results, JumpListVersion::kNext);
     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)
-    update_results->update_timeout = true;
-
   // Update the "Tasks" category of the JumpList.
-  if (!UpdateTaskCategory(&jumplist_updater, incognito_availability))
+  if (!UpdateTaskCategory(&jumplist_updater, incognito_availability)) {
+    DeleteIconFilesUnified(most_visited_icon_dir, recent_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 (update_results->most_visited_should_update)
+      update_results->most_visited_icons.swap(most_visited_icons_next);
+    if (update_results->recent_closed_should_update)
+      update_results->recent_closed_icons.swap(recent_closed_icons_next);
+  }
+
+  // Delete the set of icons based on commit status.
+  JumpListVersion version =
+      commit_success ? JumpListVersion::kCurrent : JumpListVersion::kNext;
+
+  DeleteIconFilesUnified(most_visited_icon_dir, recent_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 +819,83 @@ 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)
+    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(
+    const base::FilePath& most_visited_icon_dir,
+    const base::FilePath& recent_closed_icon_dir,
+    UpdateResults* update_results,
+    JumpListVersion version) {
+  DCHECK(update_results);
+
+  if (update_results->most_visited_should_update) {
+    DeleteIconFiles(most_visited_icon_dir,
+                    &update_results->most_visited_icon_assoc, version);
+  }
+  if (update_results->recent_closed_should_update) {
+    DeleteIconFiles(recent_closed_icon_dir,
+                    &update_results->recent_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);
 }