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 163 matching lines @@
   }
 
   return jumplist_updater->AddTasks(items);
 }
 
 // Updates the application JumpList.
 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
   // of Windows.
   if (!JumpListUpdater::IsEnabled())
     return true;
 
   JumpListUpdater jumplist_updater(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.
+  // 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.

+  // 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.
-  // 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
-  const int kMostVisited = 60;
-  const int kRecentlyClosed = 40;
+
+  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 (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);
 
-  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 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.
-    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"));
 
-    // Create icon files for shortcuts in the "Recently Closed" category.
-    CreateIconFiles(icon_dir, recently_closed_pages, recently_closed_items);
+    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),
@@ skipping 16 matching lines @@
   if (!jumplist_updater.CommitUpdate())
     return false;
 
   return true;
 }
 
 // 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_;
   }
 
   // Create a new JumpList and replace the current JumpList with it. The
   // 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
 
 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 61 matching lines @@
     pref_change_registrar_.reset();
   }
   profile_ = NULL;
 }
 
 void JumpList::ShutdownOnUIThread() {
   DCHECK(CalledOnValidThread());
   Terminate();
 }
 
 void JumpList::OnMostVisitedURLsAvailable(

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

It seems that while the set of most visited sites may change often, there should
be a lot of overlap in each update. The same may be true for recently closed. In
the interest of reducing IO, it may make sense to cache the mapping of favicon
identifier to image filename in the JumpList instance so that the work to fetch
favicons, delete old ones, write new ones to disk, etc can all be skipped when
not needed.

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

I agree with the idea. I'll use another CL for this as it's out of  scope of
this CL.

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

Agreed, this isn't the right CL for it. Thanks.

     const history::MostVisitedURLList& urls) {
   DCHECK(CalledOnValidThread());
   // If we have a pending favicon request, cancel it here (it is out of date).
   CancelPendingUpdate();
 
   {
     JumpListData* data = &jumplist_data_->data;
     base::AutoLock auto_lock(data->list_lock_);
     data->most_visited_pages_.clear();
     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));

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

nit: std::make_pair

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

Done.

     }
   }
 
+  // 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();
 }
 
 void JumpList::TabRestoreServiceChanged(sessions::TabRestoreService* service) {
   DCHECK(CalledOnValidThread());
   // if we have a pending handle request, cancel it here (it is out of date).

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

nit: "handle" -> "favicon"

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

Done.

   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 = 4;
+  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),

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

this looks pretty messed up: AddTab puts ShellLinkItems into temp_list, but puts
icon urls directly into this->data_ (acquiring and releasing the mutex each
time). this can lead to data inconsistency. it seems much, much more reliable to
apply both changes to this->data atomically (as is done in
OnMostVisitedURLsAvailable).

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

Done. Now JumpListData->data is updated entirely atomically.

                &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;

[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();
 }
 
 void JumpList::TabRestoreServiceDestroyed(
     sessions::TabRestoreService* service) {}
 
 bool JumpList::AddTab(const sessions::TabRestoreService::Tab& tab,
                       ShellLinkItemList* list,
                       size_t max_items) {
@@ skipping 99 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,
                  base::TimeDelta::FromMilliseconds(kDelayForJumplistUpdateInMS),

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

It seems that favicons are fetched as the most visited and recently closed lists
are updated. These will be dropped on the floor if another similar update comes
in within the kDelayForJumplistUpdate window, no? Would there not potentially be
fewer favicon fetches if this fetching were moved to the end of this window?

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

Yes, those favicons will be thrown away if another similar update comes within
the kDelayForJumplistUpdate window. Moving the favicon fetching step involves
quite a bit of code reordering and refactoring IMHO and it's beyond the scope of
this CL anyway. I'll use another CL for this change too. 

                  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);
 }