Improve JumpList icon folders' naming style

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 a9e27e77aa8de0baf1e25e1aa45957d14cd94f49..9104c7fb8ae316ee45abed9228843d02ee367ee3 100644
--- a/chrome/browser/win/jumplist.cc
+++ b/chrome/browser/win/jumplist.cc
@@ -197,13 +197,22 @@ bool UpdateTaskCategory(
   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,
@@ -261,8 +270,9 @@ bool UpdateJumpList(const wchar_t* app_id,
 
   // 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);
@@ -272,8 +282,9 @@ bool UpdateJumpList(const wchar_t* app_id,
 
   // 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);
@@ -315,7 +326,7 @@ bool UpdateJumpList(const wchar_t* app_id,
 // 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;
@@ -350,7 +361,7 @@ void RunUpdateJumpList(IncognitoModePrefs::Availability incognito_availability,
   // 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_);
@@ -400,7 +411,6 @@ JumpList::JumpList(Profile* profile)
 
   app_id_ =
       shell_integration::win::GetChromiumModelIdForProfile(profile_->GetPath());
-  icon_dir_ = profile_->GetPath().Append(chrome::kJumpListIconDirname);
 
   scoped_refptr<history::TopSites> top_sites =
       TopSitesFactory::GetForProfile(profile_);
@@ -680,22 +690,26 @@ void JumpList::DeferredRunUpdate() {
       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));