Update different JumpList categories on demand

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 461c5a4a45b5371c7c64ff66d79060b35a5c7bdd..cd3dbaec56c43925d29adafe0ebb61ceb387d02c 100644
--- a/chrome/browser/win/jumplist.cc
+++ b/chrome/browser/win/jumplist.cc
@@ -181,6 +181,8 @@ bool UpdateJumpList(const wchar_t* app_id,
                     const base::FilePath& icon_dir,
                     const ShellLinkItemList& most_visited_pages,
                     const ShellLinkItemList& recently_closed_pages,
+                    bool should_update_most_visited,
+                    bool should_update_recent_closed,
                     IncognitoModePrefs::Availability incognito_availability) {
   // JumpList is implemented only on Windows 7 or later.
   // So, we should return now when this function is called on earlier versions
@@ -192,43 +194,74 @@ bool UpdateJumpList(const wchar_t* app_id,
   if (!jumplist_updater.BeginUpdate())
     return false;
 
-  // We allocate 60% of the given JumpList slots to "most-visited" items
-  // and 40% to "recently-closed" items, respectively.
-  // Nevertheless, if there are not so many items in |recently_closed_pages|,
-  // we give the remaining slots to "most-visited" items.
   // The default maximum number of items to display in JumpList is 10.
   // https://msdn.microsoft.com/en-us/library/windows/desktop/dd378398(v=vs.85).aspx

[grt (UTC plus 2), 2017/04/25 08:29:39]

hot tip: this can be reduced to
https://msdn.microsoft.com/library/windows/desktop/dd378398.aspx

[chengx, 2017/04/25 23:00:24]

Thanks! I've moved the change of this section to a separate CL
crrev.com/2844463003/, which is also updated according to your suggestion.

-  const int kMostVisited = 60;
-  const int kRecentlyClosed = 40;
+  // The "Most visited" category title always takes 1 of the JumpList slots if
+  // |most_visited_pages| isn't empty.
+  // The "Recently closed" category title will also take 1 if
+  // |recently_closed_pages| isn't empty.
+  // For the remaining slots, we allocate 5/8 (i.e., 5 slots if both categories
+  // present) to "most-visited" items and 3/8 (i.e., 3 slots if both categories
+  // present) to "recently-closed" items, respectively.
+  // Nevertheless, if there are not so many items in |recently_closed_pages|,
+  // we give the remaining slots to "most-visited" items.
+
+  const int kMostVisited = 50;
+  const int kRecentlyClosed = 30;
   const int kTotal = kMostVisited + kRecentlyClosed;
+
+  // Adjust the available jumplist slots accordingly.
+  size_t user_max_items_adjusted = jumplist_updater.user_max_items();
+  if (!most_visited_pages.empty())
+    user_max_items_adjusted--;
+  if (!recently_closed_pages.empty())
+    user_max_items_adjusted--;
+
   size_t most_visited_items =
-      MulDiv(jumplist_updater.user_max_items(), kMostVisited, kTotal);
-  size_t recently_closed_items =
-      jumplist_updater.user_max_items() - most_visited_items;
+      MulDiv(user_max_items_adjusted, kMostVisited, kTotal);
+  size_t recently_closed_items = user_max_items_adjusted - most_visited_items;
   if (recently_closed_pages.size() < recently_closed_items) {
     most_visited_items += recently_closed_items - recently_closed_pages.size();
     recently_closed_items = recently_closed_pages.size();
   }
 
-  // Delete the content in JumpListIcons folder and log the results to UMA.
-  DeleteDirectoryContentAndLogResults(icon_dir, kFileDeleteLimit);
-
-  // If JumpListIcons directory doesn't exist (we have tried to create it
-  // already) or is not empty, skip updating the jumplist icons. The jumplist
-  // links should be updated anyway, as it doesn't involve disk IO. In this
-  // case, Chrome's icon will be used for the new links.
+  if (should_update_most_visited) {
+    // Delete the content in JumpListIconsMostVisited folder and log the results
+    // to UMA.
+    base::FilePath icon_dir_most_visited = icon_dir.DirName().Append(
+        icon_dir.BaseName().value() + FILE_PATH_LITERAL("MostVisited"));
+
+    DeleteDirectoryContentAndLogResults(icon_dir_most_visited,
+                                        kFileDeleteLimit);
+
+    // If the directory doesn't exist (we have tried to create it in
+    // DeleteDirectoryContentAndLogResults) or is not empty, skip updating the
+    // jumplist icons. The jumplist links should be updated anyway, as it
+    // doesn't involve disk IO. In this case, Chrome's icon will be used for the
+    // new links.
+    if (base::DirectoryExists(icon_dir_most_visited) &&
+        base::IsDirectoryEmpty(icon_dir_most_visited)) {
+      // Create icon files for shortcuts in the "Most Visited" category.
+      CreateIconFiles(icon_dir_most_visited, most_visited_pages,
+                      most_visited_items);
+    }
+  }
 
-  if (base::DirectoryExists(icon_dir) && base::IsDirectoryEmpty(icon_dir)) {
-    // TODO(chengx): Remove this UMA metric after fixing http://crbug.com/40407.
-    UMA_HISTOGRAM_COUNTS_100(
-        "WinJumplist.CreateIconFilesCount",
-        most_visited_pages.size() + recently_closed_pages.size());
+  if (should_update_recent_closed) {
+    // Delete the content in JumpListIconsRecentClosed folder and log the
+    // results to UMA.
+    base::FilePath icon_dir_recent_closed = icon_dir.DirName().Append(
+        icon_dir.BaseName().value() + FILE_PATH_LITERAL("RecentClosed"));
 
-    // Create icon files for shortcuts in the "Most Visited" category.
-    CreateIconFiles(icon_dir, most_visited_pages, most_visited_items);
+    DeleteDirectoryContentAndLogResults(icon_dir_recent_closed,
+                                        kFileDeleteLimit);
 
-    // Create icon files for shortcuts in the "Recently Closed" category.
-    CreateIconFiles(icon_dir, recently_closed_pages, recently_closed_items);
+    if (base::DirectoryExists(icon_dir_recent_closed) &&
+        base::IsDirectoryEmpty(icon_dir_recent_closed)) {
+      // Create icon files for shortcuts in the "Recently Closed" category.
+      CreateIconFiles(icon_dir_recent_closed, recently_closed_pages,
+                      recently_closed_items);
+    }
   }
 
   // TODO(chengx): Remove the UMA histogram after fixing http://crbug.com/40407.
@@ -265,6 +298,8 @@ bool UpdateJumpList(const wchar_t* app_id,
 void RunUpdateJumpList(IncognitoModePrefs::Availability incognito_availability,
                        const std::wstring& app_id,
                        const base::FilePath& icon_dir,
+                       bool should_update_most_visited,
+                       bool should_update_recent_closed,
                        base::RefCountedData<JumpListData>* ref_counted_data) {
   JumpListData* data = &ref_counted_data->data;
   ShellLinkItemList local_most_visited_pages;
@@ -286,7 +321,8 @@ void RunUpdateJumpList(IncognitoModePrefs::Availability incognito_availability,
   // jumplist links are updated anyway, while the jumplist icons may not as
   // mentioned above.
   UpdateJumpList(app_id.c_str(), icon_dir, local_most_visited_pages,
-                 local_recently_closed_pages, incognito_availability);
+                 local_recently_closed_pages, should_update_most_visited,
+                 should_update_recent_closed, incognito_availability);
 }
 
 }  // namespace
@@ -299,15 +335,17 @@ JumpList::JumpList(Profile* profile)
     : RefcountedKeyedService(content::BrowserThread::GetTaskRunnerForThread(
           content::BrowserThread::UI)),
       profile_(profile),
+      should_update_most_visited_(true),
+      should_update_recent_closed_(true),
       jumplist_data_(new base::RefCountedData<JumpListData>),
       task_id_(base::CancelableTaskTracker::kBadTaskId),
-      update_jumplisticons_task_runner_(base::CreateCOMSTATaskRunnerWithTraits(
+      update_jumplist_task_runner_(base::CreateCOMSTATaskRunnerWithTraits(
           base::TaskTraits()
               .WithPriority(base::TaskPriority::USER_VISIBLE)
               .WithShutdownBehavior(
                   base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN)
               .MayBlock())),
-      delete_jumplisticonsold_task_runner_(
+      delete_jumplisticons_task_runner_(
           base::CreateSequencedTaskRunnerWithTraits(
               base::TaskTraits()
                   .WithPriority(base::TaskPriority::BACKGROUND)
@@ -413,6 +451,11 @@ void JumpList::OnMostVisitedURLsAvailable(
     }
   }
 
+  // Since only the "Most Visited" category changes, mark the flags so that icon
+  // files only for this category are updated later on.
+  should_update_most_visited_ = true;
+  should_update_recent_closed_ = false;

[grt (UTC plus 2), 2017/04/25 08:29:39]

what if visited URLs arrive immediately after getting tabs from the tab restore
service? won't this ignore the links/icons collected from the previous
notification? same comment in TabRestoreServiceChanged.

[chengx, 2017/04/25 23:00:24]

You're right. The "TopSites Service" and the "TabRestore Service" can overwrite
each other w.r.t these two boolean flags if their update are very close in time,
typically 200 ms or even less according to the runtime and queue time of the
jumplist update step. This rarely happens and it recovers easily if it does
happen. I was thinking to pass these two flags from here all the way down to
RunUpdateJumpList call, which could avoid mutual overwriting. But given the fact
that it rarely happens, I think making these two flags global variables is fine.
The code is also cleaner a bit due to this.

+
   // Send a query that retrieves the first favicon.
   StartLoadingFavicon();
 }
@@ -436,7 +479,7 @@ void JumpList::TabRestoreServiceChanged(sessions::TabRestoreService* service) {
   //   An empty string. This value is to be updated in OnFaviconDataAvailable().
   // This code is copied from
   // RecentlyClosedTabsHandler::TabRestoreServiceChanged() to emulate it.
-  const int kRecentlyClosedCount = 4;
+  const int kRecentlyClosedCount = 3;
   sessions::TabRestoreService* tab_restore_service =
       TabRestoreServiceFactory::GetForProfile(profile_);
   for (const auto& entry : tab_restore_service->entries()) {
@@ -459,6 +502,11 @@ void JumpList::TabRestoreServiceChanged(sessions::TabRestoreService* service) {
     data->recently_closed_pages_ = temp_list;

[grt (UTC plus 2), 2017/04/25 08:29:39]

swap would be more efficient here so that no copies are made and so the old data
from recently_closed_pages_ is deleted outside of the lock.

[chengx, 2017/04/25 23:00:24]

This piece of code is gone after making JumpListData->data update entirely
atomically.

   }
 
+  // Since only the "Recently Closed" category changes, mark the flags so that
+  // icon files only for this category are updated later on.
+  should_update_most_visited_ = false;
+  should_update_recent_closed_ = true;
+
   // Send a query that retrieves the first favicon.
   StartLoadingFavicon();
 }
@@ -578,10 +626,15 @@ void JumpList::OnIncognitoAvailabilityChanged() {
     base::AutoLock auto_lock(data->list_lock_);
     waiting_for_icons = !data->icon_urls_.empty();
   }
-  if (!waiting_for_icons)
+
+  // Since neither the "Most Visited" category nor the "Recently Closed"
+  // category changes, mark the flags so that icon files for those categories
+  // won't be updated later on.
+  if (!waiting_for_icons) {
+    should_update_most_visited_ = false;
+    should_update_recent_closed_ = false;
     PostRunUpdate();
-  // If |icon_urls_| isn't empty then OnFaviconDataAvailable will eventually
-  // call PostRunUpdate().
+  }
 }
 
 void JumpList::PostRunUpdate() {
@@ -611,17 +664,26 @@ void JumpList::DeferredRunUpdate() {
       profile_ ? IncognitoModePrefs::GetAvailability(profile_->GetPrefs())
                : IncognitoModePrefs::ENABLED;
 
-  // Post a task to update the jumplist in JumpListIcons folder, which consists
-  // of 1) delete old icons, 2) create new icons, 3) notify the OS.
-  update_jumplisticons_task_runner_->PostTask(
-      FROM_HERE, base::Bind(&RunUpdateJumpList, incognito_availability, app_id_,
-                            icon_dir_, base::RetainedRef(jumplist_data_)));
-
-  // Post a task to delete JumpListIconsOld folder and log the results to UMA.
+  // Post a task to update the JumpList, which consists of 1) delete old icons,
+  // 2) create new icons, 3) notify the OS.
+  update_jumplist_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&RunUpdateJumpList, incognito_availability, app_id_, icon_dir_,
+                 should_update_most_visited_, should_update_recent_closed_,
+                 base::RetainedRef(jumplist_data_)));
+
+  // Post a task to delete JumpListIcons folder as it't no longer needed.
+  // Now we have JumpListIconsMostVisited folder and JumpListIconsRecentClosed
+  // folder instead.
+  delete_jumplisticons_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&DeleteDirectoryAndLogResults, icon_dir_, kFileDeleteLimit));
+
+  // Post a task to delete JumpListIconsOld folder as it't no longer needed.
   base::FilePath icon_dir_old = icon_dir_.DirName().Append(
       icon_dir_.BaseName().value() + FILE_PATH_LITERAL("Old"));
 
-  delete_jumplisticonsold_task_runner_->PostTask(
+  delete_jumplisticons_task_runner_->PostTask(
       FROM_HERE, base::Bind(&DeleteDirectoryAndLogResults,
                             std::move(icon_dir_old), kFileDeleteLimit));
 }