Change 4 funcs to JumpList member funcs, retire wstring for jumplist*

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 e6b2a4fdf206547a227b46b7ec2d947be6a4e24b..12e2d2de72ee1b27aa837f8c97b2bdecee837518 100644
--- a/chrome/browser/win/jumplist.cc
+++ b/chrome/browser/win/jumplist.cc
@@ -127,33 +127,6 @@ bool CreateIconFile(const gfx::ImageSkia& image_skia,
   return true;
 }
 
-// Creates icon files for the asynchrounously loaded icons.
-void CreateIconFiles(const base::FilePath& icon_dir,
-                     const ShellLinkItemList& item_list,
-                     size_t max_items) {
-  // TODO(chengx): Remove the UMA histogram after fixing http://crbug.com/40407.
-  SCOPED_UMA_HISTOGRAM_TIMER("WinJumplist.CreateIconFilesDuration");
-
-  for (ShellLinkItemList::const_iterator item = item_list.begin();
-       item != item_list.end() && max_items > 0; ++item, --max_items) {
-    base::FilePath icon_path;
-    if (CreateIconFile((*item)->icon_image(), icon_dir, &icon_path))
-      (*item)->set_icon(icon_path.value(), 0);
-  }
-}
-
-// Updates icon files in |icon_dir|, which includes deleting old icons and
-// creating at most |slot_limit| new icons for |page_list|.
-void UpdateIconFiles(const base::FilePath& icon_dir,
-                     const ShellLinkItemList& page_list,
-                     size_t slot_limit) {
-  DeleteDirectoryContentAndLogRuntime(icon_dir, kFileDeleteLimit);
-
-  // Create new icons only when the directory exists and is empty.
-  if (base::CreateDirectory(icon_dir) && base::IsDirectoryEmpty(icon_dir))
-    CreateIconFiles(icon_dir, page_list, slot_limit);
-}
-
 // Updates the "Tasks" category of the JumpList.
 bool UpdateTaskCategory(
     JumpListUpdater* jumplist_updater,
@@ -207,170 +180,6 @@ base::FilePath GenerateJumplistIconDirName(
   return profile_dir.Append(dir_name);
 }
 
-// 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.
-bool UpdateJumpList(const wchar_t* app_id,
-                    const base::FilePath& profile_dir,
-                    const ShellLinkItemList& most_visited_pages,
-                    const ShellLinkItemList& recently_closed_pages,
-                    bool most_visited_pages_have_updates,
-                    bool recently_closed_pages_have_updates,
-                    IncognitoModePrefs::Availability incognito_availability) {
-  if (!JumpListUpdater::IsEnabled())
-    return true;
-
-  JumpListUpdater jumplist_updater(app_id);
-
-  base::ElapsedTimer begin_update_timer;
-
-  if (!jumplist_updater.BeginUpdate())
-    return false;
-
-  // 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.
-  if (begin_update_timer.Elapsed() >= kTimeOutForJumplistUpdate)
-    return false;
-
-  // 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();
-  }
-
-  // Record the desired number of icons to create in this JumpList update.
-  int icons_to_create = 0;
-
-  // Update the icons for "Most Visisted" category of the JumpList if needed.
-  if (most_visited_pages_have_updates) {
-    base::FilePath icon_dir_most_visited = GenerateJumplistIconDirName(
-        profile_dir, FILE_PATH_LITERAL("MostVisited"));
-
-    UpdateIconFiles(icon_dir_most_visited, most_visited_pages,
-                    most_visited_items);
-
-    icons_to_create += std::min(most_visited_pages.size(), most_visited_items);
-  }
-
-  // Update the icons for "Recently Closed" category of the JumpList if needed.
-  if (recently_closed_pages_have_updates) {
-    base::FilePath icon_dir_recent_closed = GenerateJumplistIconDirName(
-        profile_dir, FILE_PATH_LITERAL("RecentClosed"));
-
-    UpdateIconFiles(icon_dir_recent_closed, recently_closed_pages,
-                    recently_closed_items);
-
-    icons_to_create +=
-        std::min(recently_closed_pages.size(), recently_closed_items);
-  }
-
-  // TODO(chengx): Remove the UMA histogram after fixing http://crbug.com/40407.
-  UMA_HISTOGRAM_COUNTS_100("WinJumplist.CreateIconFilesCount", icons_to_create);
-
-  // 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 false;
-  }
-
-  // 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 false;
-  }
-
-  // Update the "Tasks" category of the JumpList.
-  if (!UpdateTaskCategory(&jumplist_updater, incognito_availability))
-    return false;
-
-  // Commit this transaction and send the updated JumpList to Windows.
-  return 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& profile_dir,
-                       base::RefCountedData<JumpListData>* ref_counted_data) {
-  JumpListData* data = &ref_counted_data->data;
-  ShellLinkItemList local_most_visited_pages;
-  ShellLinkItemList local_recently_closed_pages;
-  bool most_visited_pages_have_updates;
-  bool recently_closed_pages_have_updates;
-
-  {
-    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 and flags so we can release the lock.
-    local_most_visited_pages = data->most_visited_pages_;
-    local_recently_closed_pages = data->recently_closed_pages_;
-
-    most_visited_pages_have_updates = data->most_visited_pages_have_updates_;
-    recently_closed_pages_have_updates =
-        data->recently_closed_pages_have_updates_;
-
-    // Clear the flags to reflect that we'll take actions on these updates.
-    data->most_visited_pages_have_updates_ = false;
-    data->recently_closed_pages_have_updates_ = false;
-  }
-
-  if (!most_visited_pages_have_updates && !recently_closed_pages_have_updates)
-    return;
-
-  // Update the application JumpList. If it fails, reset the flags to true if
-  // they were so that the corresponding JumpList categories will be tried to
-  // update again in the next run.
-  if (!UpdateJumpList(
-          app_id.c_str(), profile_dir, local_most_visited_pages,
-          local_recently_closed_pages, most_visited_pages_have_updates,
-          recently_closed_pages_have_updates, incognito_availability)) {
-    base::AutoLock auto_lock(data->list_lock_);
-    if (most_visited_pages_have_updates)
-      data->most_visited_pages_have_updates_ = true;
-    if (recently_closed_pages_have_updates)
-      data->recently_closed_pages_have_updates_ = true;
-  }
-}
-
 }  // namespace
 
 JumpList::JumpListData::JumpListData() {}
@@ -668,8 +477,10 @@ void JumpList::PostRunUpdate() {
   // 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_,
-                            profile_dir, base::RetainedRef(jumplist_data_)));
+      FROM_HERE,
+      base::Bind(&JumpList::RunUpdateJumpList, base::Unretained(this),

[gab, 2017/05/09 14:54:28]

Looks like jumplist is owned by BrowserView [1]. As such base::Unretained is
inappropriate as it will crash if the BrowserView is destroyed while this task
is in the queue.

What do you the outcome to be when the BrowserView is destroyed? Cancel the
pending tasks or keep Jumplist alive until its tasks complete?

JumpList is RefCounted but there doesn't appear to be a good reason for that (it
has a single owner). If cancellation is appropriate then let's change it to not
be RefCounted (i.e. be owned via a std::unique_ptr) and add a WeakPtrFactory
member which can be used to weakly bind tasks (will be cancelled if Jumplist is
destroyed early).

Note however that the current behavior (per the anonymous tasks) is to keep
running after the BrowserView goes away.

[1]
https://cs.chromium.org/chromium/src/chrome/browser/ui/views/frame/browser_vi...

[chengx, 2017/05/09 18:04:14]

Thanks for pointing me to this, and I'm happy to fix it. How about this? I've
filed crbug.com/720028 for this, and I'll use another CL for this purpose. It's
out of the scope of this CL anyway IMHO.

[gab, 2017/05/09 18:37:36]

Well you can't leave Unretained(this) in this CL though, it will crash if
BrowserView is destroyed while the task is in the queue.

You need to just pass |this| and force an extra ref to be bound to keep |this|
alive until the callback runs.
Looks like this is already done in at least one place [1] in the codebase, which
might be why it's currently refcounted -- but that callsite could perhaps also
use  WeakPtr eventually?

[1]
https://cs.chromium.org/chromium/src/chrome/browser/win/jumplist.cc?type=cs&q...

[chengx, 2017/05/09 18:56:03]

Oh, I see. I've changed base::Unretained(this) to |this|. I am not sure what it
means by "force an extra ref to be bound to keep |this| alive ... " though. Do I
do need to make more changes? 

+                 incognito_availability, app_id_, profile_dir,
+                 base::RetainedRef(jumplist_data_)));
 
   // Post a task to delete JumpListIcons folder as it's no longer needed.
   // Now we have JumpListIconsMostVisited folder and JumpListIconsRecentClosed
@@ -764,3 +575,187 @@ void JumpList::DeferredTabRestoreServiceChanged() {
   // Send a query that retrieves the first favicon.
   StartLoadingFavicon();
 }
+
+void JumpList::CreateIconFiles(const base::FilePath& icon_dir,
+                               const ShellLinkItemList& item_list,
+                               size_t max_items) {
+  // TODO(chengx): Remove the UMA histogram after fixing http://crbug.com/40407.
+  SCOPED_UMA_HISTOGRAM_TIMER("WinJumplist.CreateIconFilesDuration");
+
+  for (ShellLinkItemList::const_iterator item = item_list.begin();
+       item != item_list.end() && max_items > 0; ++item, --max_items) {
+    base::FilePath icon_path;
+    if (CreateIconFile((*item)->icon_image(), icon_dir, &icon_path))
+      (*item)->set_icon(icon_path.value(), 0);
+  }
+}
+
+void JumpList::UpdateIconFiles(const base::FilePath& icon_dir,
+                               const ShellLinkItemList& page_list,
+                               size_t slot_limit) {
+  DeleteDirectoryContentAndLogRuntime(icon_dir, kFileDeleteLimit);
+
+  // Create new icons only when the directory exists and is empty.
+  if (base::CreateDirectory(icon_dir) && base::IsDirectoryEmpty(icon_dir))
+    CreateIconFiles(icon_dir, page_list, slot_limit);
+}
+
+bool JumpList::UpdateJumpList(
+    const wchar_t* app_id,
+    const base::FilePath& profile_dir,
+    const ShellLinkItemList& most_visited_pages,
+    const ShellLinkItemList& recently_closed_pages,
+    bool most_visited_pages_have_updates,
+    bool recently_closed_pages_have_updates,
+    IncognitoModePrefs::Availability incognito_availability) {
+  if (!JumpListUpdater::IsEnabled())
+    return true;
+
+  JumpListUpdater jumplist_updater(app_id);

[gab, 2017/05/09 14:54:28]

Looks like this is taking a string& (so taking it by char* above was resulting
in an unnecessary string copy)

Consider updating JumpListUpdater to use string16 as well for consistency.

[chengx, 2017/05/09 18:04:14]

I've updated JumpList::UpdateJumpList to use string16& from wchar_t*. I've also
updated JumpListUpdater to use base::string16.

[gab, 2017/05/09 20:07:11]

No, refcounting is the default for Bind. Since Jumplist inherits from
base::RefCounted passing |this| makes Bind take a reference (it simply wouldn't
compile if it wasn't RefCounted).

[chengx, 2017/05/09 20:11:31]

Cool. Thanks for the explanation!

+
+  base::ElapsedTimer begin_update_timer;
+
+  if (!jumplist_updater.BeginUpdate())
+    return false;
+
+  // 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.
+  if (begin_update_timer.Elapsed() >= kTimeOutForJumplistUpdate)
+    return false;
+
+  // 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();
+  }
+
+  // Record the desired number of icons to create in this JumpList update.
+  int icons_to_create = 0;
+
+  // Update the icons for "Most Visisted" category of the JumpList if needed.
+  if (most_visited_pages_have_updates) {
+    base::FilePath icon_dir_most_visited = GenerateJumplistIconDirName(
+        profile_dir, FILE_PATH_LITERAL("MostVisited"));
+
+    UpdateIconFiles(icon_dir_most_visited, most_visited_pages,
+                    most_visited_items);
+
+    icons_to_create += std::min(most_visited_pages.size(), most_visited_items);
+  }
+
+  // Update the icons for "Recently Closed" category of the JumpList if needed.
+  if (recently_closed_pages_have_updates) {
+    base::FilePath icon_dir_recent_closed = GenerateJumplistIconDirName(
+        profile_dir, FILE_PATH_LITERAL("RecentClosed"));
+
+    UpdateIconFiles(icon_dir_recent_closed, recently_closed_pages,
+                    recently_closed_items);
+
+    icons_to_create +=
+        std::min(recently_closed_pages.size(), recently_closed_items);
+  }
+
+  // TODO(chengx): Remove the UMA histogram after fixing http://crbug.com/40407.
+  UMA_HISTOGRAM_COUNTS_100("WinJumplist.CreateIconFilesCount", icons_to_create);
+
+  // 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 false;
+  }
+
+  // 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 false;
+  }
+
+  // Update the "Tasks" category of the JumpList.
+  if (!UpdateTaskCategory(&jumplist_updater, incognito_availability))
+    return false;
+
+  // Commit this transaction and send the updated JumpList to Windows.
+  return jumplist_updater.CommitUpdate();
+}
+
+void JumpList::RunUpdateJumpList(
+    IncognitoModePrefs::Availability incognito_availability,
+    const std::wstring& app_id,
+    const base::FilePath& profile_dir,
+    base::RefCountedData<JumpListData>* ref_counted_data) {
+  JumpListData* data = &ref_counted_data->data;
+  ShellLinkItemList local_most_visited_pages;
+  ShellLinkItemList local_recently_closed_pages;
+  bool most_visited_pages_have_updates;
+  bool recently_closed_pages_have_updates;
+
+  {
+    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 and flags so we can release the lock.
+    local_most_visited_pages = data->most_visited_pages_;
+    local_recently_closed_pages = data->recently_closed_pages_;
+
+    most_visited_pages_have_updates = data->most_visited_pages_have_updates_;
+    recently_closed_pages_have_updates =
+        data->recently_closed_pages_have_updates_;
+
+    // Clear the flags to reflect that we'll take actions on these updates.
+    data->most_visited_pages_have_updates_ = false;
+    data->recently_closed_pages_have_updates_ = false;
+  }
+
+  if (!most_visited_pages_have_updates && !recently_closed_pages_have_updates)
+    return;
+
+  // Update the application JumpList. If it fails, reset the flags to true if
+  // they were so that the corresponding JumpList categories will be tried to
+  // update again in the next run.
+  if (!UpdateJumpList(
+          app_id.c_str(), profile_dir, local_most_visited_pages,
+          local_recently_closed_pages, most_visited_pages_have_updates,
+          recently_closed_pages_have_updates, incognito_availability)) {
+    base::AutoLock auto_lock(data->list_lock_);
+    if (most_visited_pages_have_updates)
+      data->most_visited_pages_have_updates_ = true;
+    if (recently_closed_pages_have_updates)
+      data->recently_closed_pages_have_updates_ = true;
+  }
+}