Update different JumpList categories on demand

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 <utility>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
@@ skipping 173 matching lines @@
 
 // Updates the application JumpList, which consists of 1) delete old icon files;
 // 2) create new icon files; 3) notify the OS.
 // Note that any timeout error along the way results in the old jumplist being
 // left as-is, while any non-timeout error results in the old jumplist being
 // left as-is, but without icon files.
 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;
 
   // Records the time cost of starting a JumpListUpdater.
   base::ElapsedTimer begin_update_timer;
 
   JumpListUpdater jumplist_updater(app_id);
   if (!jumplist_updater.BeginUpdate())
     return;
 
   // Stops jumplist update if JumpListUpdater's start times out, as it's very
   // likely the following update steps will also take a long time.
   if (begin_update_timer.Elapsed() >= kTimeOutForJumplistUpdate)
     return;
 
   // The default maximum number of items to display in JumpList is 10.
   // https://msdn.microsoft.com/library/windows/desktop/dd378398.aspx
   // 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 to account for the category titles.
   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(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 (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"));
 
-  // 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_most_visited,
+                                        kFileDeleteLimit);
 
-  bool should_create_icons =
-      base::DirectoryExists(icon_dir) && base::IsDirectoryEmpty(icon_dir);
+    // 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);
+    }
+  }
 
-  // Create icon files for shortcuts in the "Most Visited" category.
-  if (should_create_icons)
-    CreateIconFiles(icon_dir, 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"));
+
+    DeleteDirectoryContentAndLogResults(icon_dir_recent_closed,
+                                        kFileDeleteLimit);
+
+    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.
+  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
   // transaction.
   if (!jumplist_updater.AddCustomCategory(
           l10n_util::GetStringUTF16(IDS_NEW_TAB_MOST_VISITED),
           most_visited_pages, most_visited_items)) {
     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),
           recently_closed_pages, recently_closed_items)) {
     return;
   }
 
   // Update the "Tasks" category of the JumpList.
   if (!UpdateTaskCategory(&jumplist_updater, incognito_availability))
     return;
 
   // Commit this transaction and send the updated JumpList to Windows.
   jumplist_updater.CommitUpdate();
 }
 
 // Updates the jumplist, once all the data has been fetched.
 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;
   ShellLinkItemList local_recently_closed_pages;
 
   {
     base::AutoLock auto_lock(data->list_lock_);
     // Make sure we are not out of date: if icon_urls_ is not empty, then
     // another notification has been received since we processed this one
     if (!data->icon_urls_.empty())
       return;
 
     // Make local copies of lists so we can release the lock.
     local_most_visited_pages = data->most_visited_pages_;
     local_recently_closed_pages = data->recently_closed_pages_;
   }
 
   // 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
 
 JumpList::JumpListData::JumpListData() {}
 
 JumpList::JumpListData::~JumpListData() {}
 
 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)
                   .WithShutdownBehavior(
                       base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN)
                   .MayBlock())),
       weak_ptr_factory_(this) {
   DCHECK(Enabled());
   // To update JumpList when a tab is added or removed, we add this object to
   // the observer list of the TabRestoreService class.
@@ skipping 81 matching lines @@
     for (size_t i = 0; i < urls.size(); i++) {
       const history::MostVisitedURL& url = urls[i];
       scoped_refptr<ShellLinkItem> link = CreateShellLink();
       std::string url_string = url.url.spec();
       std::wstring url_string_wide = base::UTF8ToWide(url_string);
       link->GetCommandLine()->AppendArgNative(url_string_wide);
       link->GetCommandLine()->AppendSwitchASCII(
           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.
   // * arguments
   //   The last URL of the tab object.
   // * title
   //   The title of the last URL.
   // * icon
   //   An empty string. This value is to be updated in OnFaviconDataAvailable().
   // This code is copied from
   // RecentlyClosedTabsHandler::TabRestoreServiceChanged() to emulate it.
   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();
 }
 
 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();
   const sessions::SerializedNavigationEntry& current_navigation =
       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;
   }
 }
 
 void JumpList::StartLoadingFavicon() {
   DCHECK(CalledOnValidThread());
 
   GURL url;
   bool waiting_for_icons = true;
   {
@@ skipping 54 matching lines @@
 
 void JumpList::OnIncognitoAvailabilityChanged() {
   DCHECK(CalledOnValidThread());
 
   bool waiting_for_icons = true;
   {
     JumpListData* data = &jumplist_data_->data;
     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() {
   DCHECK(CalledOnValidThread());
 
   TRACE_EVENT0("browser", "JumpList::PostRunUpdate");
   // Initialize the one-shot timer to update the jumplists in a while.
   // If there is already a request queued then cancel it and post the new
   // request. This ensures that JumpListUpdates won't happen until there has
   // been a brief quiet period, thus avoiding update storms.
   if (timer_.IsRunning()) {
     timer_.Reset();
   } else {
     timer_.Start(FROM_HERE, kDelayForJumplistUpdate, this,
                  &JumpList::DeferredRunUpdate);
   }
 }
 
 void JumpList::DeferredRunUpdate() {
   DCHECK(CalledOnValidThread());
 
   TRACE_EVENT0("browser", "JumpList::DeferredRunUpdate");
   // Check if incognito windows (or normal windows) are disabled by policy.
   IncognitoModePrefs::Availability incognito_availability =
       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 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 JumpListIconsOld folder and log the results to UMA.
+  // 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));
 }
 
 void JumpList::TopSitesLoaded(history::TopSites* top_sites) {
 }
 
 void JumpList::TopSitesChanged(history::TopSites* top_sites,
                                ChangeReason change_reason) {
   top_sites->GetMostVisitedURLs(
       base::Bind(&JumpList::OnMostVisitedURLsAvailable,
                  weak_ptr_factory_.GetWeakPtr()),
       false);
 }