Delete the right JumpList icons conditional on shell notification

chrome/browser/win/jumplist.cc

 // Copyright 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/win/jumplist.h"
 
 #include "base/base_paths.h"
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/command_line.h"
@@ skipping 176 matching lines @@
 base::FilePath GenerateJumplistIconDirName(
     const base::FilePath& profile_dir,
     const base::FilePath::StringPieceType& suffix) {
   base::FilePath::StringType dir_name(chrome::kJumpListIconDirname);
   suffix.AppendToString(&dir_name);
   return profile_dir.Append(dir_name);
 }
 
 }  // namespace
 
-JumpList::UpdateResults::UpdateResults() {}
+JumpList::UpdateTransaction::UpdateTransaction() {}
 
-JumpList::UpdateResults::~UpdateResults() {}
+JumpList::UpdateTransaction::~UpdateTransaction() {}
 
 // static
 bool JumpList::Enabled() {
   return JumpListUpdater::IsEnabled();
 }
 
 void JumpList::Shutdown() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   Terminate();
 }
@@ skipping 335 matching lines @@
 
   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());
 
   // 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
   // just finished.
   if (update_results->update_timeout)
     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;
 
   // 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_ ||
@@ skipping 41 matching lines @@
     scoped_refptr<history::TopSites> top_sites =
         TopSitesFactory::GetForProfile(profile_);
     if (top_sites)
       top_sites->RemoveObserver(this);
     pref_change_registrar_.reset();
   }
   profile_ = nullptr;
 }
 
 // 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);
 
   base::ElapsedTimer begin_update_timer;
 
   if (!jumplist_updater.BeginUpdate())
     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.
+  // steps will also take a long time.
   if (begin_update_timer.Elapsed() >= kTimeOutForJumplistBeginUpdate) {
     update_results->update_timeout = true;
     return;
   }
 
   // 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.
   UMA_HISTOGRAM_COUNTS_100("WinJumplist.CreateIconFilesCount", icons_created);
 
   // TODO(chengx): Remove the UMA histogram after fixing http://crbug.com/40407.
   SCOPED_UMA_HISTOGRAM_TIMER("WinJumplist.UpdateJumpListDuration");
 
   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(
           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(
           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.
-
-  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))
     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 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;
 }
 
 // static
 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);
 }