Improve JumpList icon folders' naming style

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 179 matching lines @@
     base::ReplaceSubstringsAfterOffset(
         &incognito_title, 0, L"&", base::StringPiece16());
     incognito->set_title(incognito_title);
     incognito->set_icon(chrome_path.value(), icon_resources::kIncognitoIndex);
     items.push_back(incognito);
   }
 
   return jumplist_updater->AddTasks(items);
 }
 
+void GenerateJumplistIconDirName(
+    const base::FilePath& profile_dir,
+    const base::FilePath::StringType& dir_name_append,

[grt (UTC plus 2), 2017/05/03 10:10:41]

nit: dir_name_append -> suffix

[grt (UTC plus 2), 2017/05/03 10:10:41]

take this as a base::FilePath::StringPieceType to avoid making an extra copy

[chengx, 2017/05/03 18:36:29]

Done.

[chengx, 2017/05/03 18:36:29]

Done.

+    base::FilePath* out_dir) {

[grt (UTC plus 2), 2017/05/03 10:10:41]

we treat FilePath as a value type -- return this directly rather than take it as
an out param

[chengx, 2017/05/03 18:36:30]

Done. I thought we should avoid copying strings. This doesn't apply to FilePath?

[grt (UTC plus 2), 2017/05/04 11:41:55]

No strings are copied here. Between RVO (return value optimization) and copy
elision, the FilePath returned by Append is passed straight back to the caller.
This is more efficient that what you had before, since there's no longer an
extra empty string created by the caller and then replaced with the one returned
by Append. C++: the land of most surprise!

[chengx, 2017/05/04 19:36:11]

Thanks for the explanation!

+  base::FilePath::StringType jumplisticon_base_dir(

[grt (UTC plus 2), 2017/05/03 10:10:41]

how about:
  base::FilePath::StringType dir_name(chrome::kJumpListIconDirname);
  suffix.AppendToString(&dir_name);
  return profile_dir.Append(dir_name);

[chengx, 2017/05/03 18:36:29]

Accepted. Thanks!

+      chrome::kJumpListIconDirname);
+  *out_dir = profile_dir.Append(jumplisticon_base_dir + dir_name_append);
+}
+
 // 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& icon_dir,
+                    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);
 
@@ skipping 37 matching lines @@
   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 = icon_dir.DirName().Append(
-        icon_dir.BaseName().value() + FILE_PATH_LITERAL("MostVisited"));
+    base::FilePath icon_dir_most_visited;
+    GenerateJumplistIconDirName(profile_dir, FILE_PATH_LITERAL("MostVisited"),
+                                &icon_dir_most_visited);
 
     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 = icon_dir.DirName().Append(
-        icon_dir.BaseName().value() + FILE_PATH_LITERAL("RecentClosed"));
+    base::FilePath icon_dir_recent_closed;
+    GenerateJumplistIconDirName(profile_dir, FILE_PATH_LITERAL("RecentClosed"),
+                                &icon_dir_recent_closed);
 
     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);
@@ skipping 21 matching lines @@
   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& icon_dir,
+                       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
@@ skipping 14 matching lines @@
     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(), icon_dir, local_most_visited_pages,
+          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;
   }
 }
 
@@ skipping 29 matching lines @@
   // When we add this object to the observer list, we save the pointer to this
   // TabRestoreService object. This pointer is used when we remove this object
   // from the observer list.
   sessions::TabRestoreService* tab_restore_service =
       TabRestoreServiceFactory::GetForProfile(profile_);
   if (!tab_restore_service)
     return;
 
   app_id_ =
       shell_integration::win::GetChromiumModelIdForProfile(profile_->GetPath());
-  icon_dir_ = profile_->GetPath().Append(chrome::kJumpListIconDirname);
 
   scoped_refptr<history::TopSites> top_sites =
       TopSitesFactory::GetForProfile(profile_);
   if (top_sites) {
     // TopSites updates itself after a delay. This is especially noticable when
     // your profile is empty. Ask TopSites to update itself when jumplist is
     // initialized.
     top_sites->SyncWithHistory();
     // Register as TopSitesObserver so that we can update ourselves when the
     // TopSites changes.
@@ skipping 21 matching lines @@
   DCHECK(CalledOnValidThread());
   if (task_id_ != base::CancelableTaskTracker::kBadTaskId) {
     cancelable_task_tracker_.TryCancel(task_id_);
     task_id_ = base::CancelableTaskTracker::kBadTaskId;
   }
 }
 
 void JumpList::Terminate() {
   DCHECK(CalledOnValidThread());
   CancelPendingUpdate();
   if (profile_) {

[grt (UTC plus 2), 2017/05/03 10:10:41]

should this also stop the timer?

[chengx, 2017/05/03 18:36:30]

It should. I'll fix this in the "delay and coalesce" CL. Thanks!

     sessions::TabRestoreService* tab_restore_service =
         TabRestoreServiceFactory::GetForProfile(profile_);
     if (tab_restore_service)
       tab_restore_service->RemoveObserver(this);
     scoped_refptr<history::TopSites> top_sites =
         TopSitesFactory::GetForProfile(profile_);
     if (top_sites)
       top_sites->RemoveObserver(this);
     pref_change_registrar_.reset();
   }
@@ skipping 217 matching lines @@
 
 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;
 
+  base::FilePath profile_dir = profile_->GetPath();

[grt (UTC plus 2), 2017/05/03 10:10:41]

null check on profile_ as above? i suppose that's there because of the clearing
in Terminate.

[chengx, 2017/05/03 18:36:29]

Yes. A null check is needed here. 

+
   // 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_, base::RetainedRef(jumplist_data_)));
+                            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
   // folder instead.
   delete_jumplisticons_task_runner_->PostTask(
-      FROM_HERE, base::Bind(&DeleteDirectory, icon_dir_, kFileDeleteLimit));
+      FROM_HERE, base::Bind(&DeleteDirectory,
+                            profile_dir.Append(chrome::kJumpListIconDirname),
+                            kFileDeleteLimit));
 
   // Post a task to delete JumpListIconsOld folder as it's no longer needed.
-  base::FilePath icon_dir_old = icon_dir_.DirName().Append(
-      icon_dir_.BaseName().value() + FILE_PATH_LITERAL("Old"));
-
+  base::FilePath icon_dir_old;
+  GenerateJumplistIconDirName(profile_dir, FILE_PATH_LITERAL("Old"),
+                              &icon_dir_old);
   delete_jumplisticons_task_runner_->PostTask(
       FROM_HERE,
       base::Bind(&DeleteDirectory, 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);
 }