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 54d4fb989cacb1a56e5e6e63197ca69ff00df41f..2c987f3a0e9551ad72312959c9d3b201034b7978 100644
--- a/chrome/browser/win/jumplist.cc
+++ b/chrome/browser/win/jumplist.cc
@@ -191,6 +191,8 @@ void 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) {
   if (!JumpListUpdater::IsEnabled())
     return;
@@ -216,6 +218,8 @@ void UpdateJumpList(const wchar_t* app_id,
   // 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;
@@ -236,20 +240,47 @@ void UpdateJumpList(const wchar_t* app_id,
     recently_closed_items = recently_closed_pages.size();
   }
 
-  // Delete the content in JumpListIcons folder and log the results to UMA.
-  DeleteDirectoryContentAndLogResults(icon_dir, kFileDeleteLimit);
+  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 (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"));
 
-  // 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.
+    DeleteDirectoryContentAndLogResults(icon_dir_recent_closed,
+                                        kFileDeleteLimit);
 
-  bool should_create_icons =
-      base::DirectoryExists(icon_dir) && base::IsDirectoryEmpty(icon_dir);
+    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);
+    }
+  }
 
-  // Create icon files for shortcuts in the "Most Visited" category.
-  if (should_create_icons)
-    CreateIconFiles(icon_dir, most_visited_pages, most_visited_items);
+  // TODO(chengx): Remove the UMA histogram after fixing http://crbug.com/40407.
+  SCOPED_UMA_HISTOGRAM_TIMER("WinJumplist.UpdateJumpListDuration");
 
   // Update the "Most Visited" category of the JumpList if it exists.
   // This update request is applied into the JumpList when we commit this
@@ -260,10 +291,6 @@ void UpdateJumpList(const wchar_t* app_id,
     return;
   }
 
-  // Create icon files for shortcuts in the "Recently Closed" category.
-  if (should_create_icons)
-    CreateIconFiles(icon_dir, recently_closed_pages, recently_closed_items);
-
   // Update the "Recently Closed" category of the JumpList.
   if (!jumplist_updater.AddCustomCategory(
           l10n_util::GetStringUTF16(IDS_RECENTLY_CLOSED),
@@ -283,6 +310,8 @@ void 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;
@@ -302,7 +331,8 @@ void RunUpdateJumpList(IncognitoModePrefs::Availability incognito_availability,
 
   // Updates the application JumpList.
   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
@@ -315,15 +345,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)
@@ -425,22 +457,24 @@ void JumpList::OnMostVisitedURLsAvailable(
           switches::kWinJumplistAction, jumplist::kMostVisitedCategory);
       link->set_title(!url.title.empty() ? url.title : url_string_wide);
       data->most_visited_pages_.push_back(link);
-      data->icon_urls_.push_back(make_pair(url_string, link));
+      data->icon_urls_.push_back(std::make_pair(url_string, link));
     }
   }
 
+  // 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;
+
   // Send a query that retrieves the first favicon.
   StartLoadingFavicon();
 }
 
 void JumpList::TabRestoreServiceChanged(sessions::TabRestoreService* service) {
   DCHECK(CalledOnValidThread());
-  // if we have a pending handle request, cancel it here (it is out of date).
+  // if we have a pending favicon request, cancel it here (it is out of date).
   CancelPendingUpdate();
 
-  // local list to pass to methods
-  ShellLinkItemList temp_list;
-
   // Create a list of ShellLinkItems from the "Recently Closed" pages.
   // As noted above, we create a ShellLinkItem objects with the following
   // parameters.
@@ -455,26 +489,31 @@ void JumpList::TabRestoreServiceChanged(sessions::TabRestoreService* service) {
   const int kRecentlyClosedCount = 3;
   sessions::TabRestoreService* tab_restore_service =
       TabRestoreServiceFactory::GetForProfile(profile_);
-  for (const auto& entry : tab_restore_service->entries()) {
-    switch (entry->type) {
-      case sessions::TabRestoreService::TAB:
-        AddTab(static_cast<const sessions::TabRestoreService::Tab&>(*entry),
-               &temp_list, kRecentlyClosedCount);
-        break;
-      case sessions::TabRestoreService::WINDOW:
-        AddWindow(
-            static_cast<const sessions::TabRestoreService::Window&>(*entry),
-            &temp_list, kRecentlyClosedCount);
-        break;
-    }
-  }
-  // Lock recently_closed_pages and copy temp_list into it.
   {
     JumpListData* data = &jumplist_data_->data;
     base::AutoLock auto_lock(data->list_lock_);
-    data->recently_closed_pages_ = temp_list;
+    data->recently_closed_pages_.clear();
+
+    for (const auto& entry : tab_restore_service->entries()) {
+      switch (entry->type) {
+        case sessions::TabRestoreService::TAB:
+          AddTab(static_cast<const sessions::TabRestoreService::Tab&>(*entry),
+                 data, kRecentlyClosedCount);
+          break;
+        case sessions::TabRestoreService::WINDOW:
+          AddWindow(
+              static_cast<const sessions::TabRestoreService::Window&>(*entry),
+              data, kRecentlyClosedCount);
+          break;
+      }
+    }
   }
 
+  // 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;

[grt (UTC plus 2), 2017/04/26 09:14:33]

do i understand correctly that the point of these two is to convey to
UpdateJumpList that the corresponding list should be processed? it seems cleaner
to put an "is dirty" bit in JumpListData next to each list that is set to true
when the list is modified (OnMostVisitedURLsAvailable and
TabRestoreServiceChanged) and then have RunUpdateJumpList extract the dirty bits
when it copies the lists themselves and clear the dirty bits. if the update
fails, you will want a way to re-apply the dirty bits to JumpListData. so maybe
UpdateJumpList should return bool afterall so that RunUpdateJumpList could do
something like:

  if (!UpdateJumpList(...)) {
    AutoLock l;
    if (most_visited_pages_dirty)
      data->most_visited_pages_dirty);
    if (local_re....
  }

i think this would avoid the problem where one type of update messes with the
state for the other.

[chengx, 2017/04/27 01:18:47]

Awesome idea! I've updated the code accordingly. Thanks!

+  should_update_recent_closed_ = true;
+
   // Send a query that retrieves the first favicon.
   StartLoadingFavicon();
 }
@@ -483,13 +522,12 @@ void JumpList::TabRestoreServiceDestroyed(
     sessions::TabRestoreService* service) {}
 
 bool JumpList::AddTab(const sessions::TabRestoreService::Tab& tab,
-                      ShellLinkItemList* list,
+                      JumpListData* data,
                       size_t max_items) {
   DCHECK(CalledOnValidThread());
 

[grt (UTC plus 2), 2017/04/26 09:14:33]

  data->list_lock_.AssertAcquired();

[chengx, 2017/04/27 01:18:47]

Done.

-  // This code adds the URL and the title strings of the given tab to the
-  // specified list.
-  if (list->size() >= max_items)
+  // This code adds the URL and the title strings of the given tab to |data|.
+  if (data->recently_closed_pages_.size() >= max_items)
     return false;
 
   scoped_refptr<ShellLinkItem> link = CreateShellLink();
@@ -497,29 +535,26 @@ bool JumpList::AddTab(const sessions::TabRestoreService::Tab& tab,
       tab.navigations.at(tab.current_navigation_index);
   std::string url = current_navigation.virtual_url().spec();
   link->GetCommandLine()->AppendArgNative(base::UTF8ToWide(url));
-  link->GetCommandLine()->AppendSwitchASCII(
-      switches::kWinJumplistAction, jumplist::kRecentlyClosedCategory);
+  link->GetCommandLine()->AppendSwitchASCII(switches::kWinJumplistAction,
+                                            jumplist::kRecentlyClosedCategory);
   link->set_title(current_navigation.title());
-  list->push_back(link);
-  {
-    JumpListData* data = &jumplist_data_->data;
-    base::AutoLock auto_lock(data->list_lock_);
-    data->icon_urls_.push_back(std::make_pair(std::move(url), std::move(link)));
-  }
+  data->recently_closed_pages_.push_back(link);
+  data->icon_urls_.push_back(std::make_pair(std::move(url), std::move(link)));
+
   return true;
 }
 
 void JumpList::AddWindow(const sessions::TabRestoreService::Window& window,
-                         ShellLinkItemList* list,
+                         JumpListData* data,
                          size_t max_items) {
   DCHECK(CalledOnValidThread());
 

[grt (UTC plus 2), 2017/04/26 09:14:33]

  data->list_lock_.AssertAcquired();

[chengx, 2017/04/27 01:18:47]

Done.

-  // This code enumerates al the tabs in the given window object and add their
-  // URLs and titles to the list.
+  // This code enumerates all the tabs in the given window object and add their
+  // URLs and titles to |data|.
   DCHECK(!window.tabs.empty());
 
   for (const auto& tab : window.tabs) {
-    if (!AddTab(*tab, list, max_items))
+    if (!AddTab(*tab, data, max_items))
       return;
   }
 }
@@ -594,10 +629,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() {
@@ -625,17 +665,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));
 }