Uniquify profile shortcut name.

chrome/browser/profiles/profile_shortcut_manager_win.cc

Index: chrome/browser/profiles/profile_shortcut_manager_win.cc
diff --git a/chrome/browser/profiles/profile_shortcut_manager_win.cc b/chrome/browser/profiles/profile_shortcut_manager_win.cc
index 0061b4cfcd4938ccc2b9094afd2968fd3708fe0a..8ea7650c17916aa9f64feef3cd08b0b2cbbd81d5 100644
--- a/chrome/browser/profiles/profile_shortcut_manager_win.cc
+++ b/chrome/browser/profiles/profile_shortcut_manager_win.cc
@@ -6,6 +6,8 @@
 
 #include <shlobj.h>  // For SHChangeNotify().
 
+#include <algorithm>
+#include <set>
 #include <string>
 #include <vector>
 
@@ -289,34 +291,57 @@ bool IsChromeShortcut(const base::FilePath& path,
   return ConvertToLongPath(target_path) == ConvertToLongPath(chrome_exe);
 }
 
-// Populates |paths| with the file paths of Chrome desktop shortcuts that have
-// the specified |command_line|. If |include_empty_command_lines| is true,
-// Chrome desktop shortcuts with empty command lines will also be included.
-void ListDesktopShortcutsWithCommandLine(const base::FilePath& chrome_exe,
-                                         const base::string16& command_line,
-                                         bool include_empty_command_lines,
-                                         std::vector<base::FilePath>* paths) {
+// Get the file paths of desktop files and folders
+// filtered by predicate |p|.

[Alexei Svitkine (slow), 2016/01/05 19:55:22]

Nit: Wrapping is off for this comment.

+template <class Filter>

[Alexei Svitkine (slow), 2016/01/05 19:55:22]

Templates are not commonly used in Chromium (outside of base/ utilities like
callback.h).

In this case, it seems the only reason you're using it so you can pass in a
filter that always returns true as a closure.

If that's the only use case, I don't think its worth bloating the binary with
two machine codes for this function (as a result of the template). Instead, pass
the filter as a const ShortcutFilenameMatcher* and allow it to be nullptr, in
which case it wouldn't be used.

+std::set<base::FilePath> ListUserDesktopContents(const Filter& p) {
+  std::set<base::FilePath> res;
+
   base::FilePath user_shortcuts_directory;
-  if (!GetDesktopShortcutsDirectories(&user_shortcuts_directory, NULL))
-    return;
+  if (!GetDesktopShortcutsDirectories(&user_shortcuts_directory, nullptr))
+    return res;
 
-  base::FileEnumerator enumerator(user_shortcuts_directory, false,
-                                  base::FileEnumerator::FILES);
+  base::FileEnumerator enumerator(
+      user_shortcuts_directory, false,
+      base::FileEnumerator::FILES | base::FileEnumerator::DIRECTORIES);
   for (base::FilePath path = enumerator.Next(); !path.empty();
        path = enumerator.Next()) {
+    if (p(path))

[Alexei Svitkine (slow), 2016/01/05 19:55:22]

p(path) doesn't sound very readable. Name the parameter more meaningfully.

+      res.insert(path);
+  }
+  return res;
+}
+
+// Checks if |path| is the Chrome desktop shortcut (|chrome_exe|) that have
+// the specified |command_line|. If |include_empty_command_lines| is true,
+// Chrome desktop shortcuts with empty command lines will also be included.
+struct ChromeCommandLineFilter {
+  const base::FilePath& chrome_exe;
+  const base::string16& command_line;
+  bool include_empty_command_lines;
+
+  ChromeCommandLineFilter(const base::FilePath& chrome_exe,
+                          const base::string16& command_line,
+                          bool include_empty_command_lines)
+      : chrome_exe(chrome_exe),
+        command_line(command_line),
+        include_empty_command_lines(include_empty_command_lines) {}
+
+  bool operator()(const base::FilePath& path) const {
     base::string16 shortcut_command_line;
     if (!IsChromeShortcut(path, chrome_exe, &shortcut_command_line))
-      continue;
+      return false;
 
     // TODO(asvitkine): Change this to build a CommandLine object and ensure all
     // args from |command_line| are present in the shortcut's CommandLine. This
     // will be more robust when |command_line| contains multiple args.
     if ((shortcut_command_line.empty() && include_empty_command_lines) ||
         (shortcut_command_line.find(command_line) != base::string16::npos)) {
-      paths->push_back(path);
+      return true;
     }
+    return false;
   }
-}
+};
 
 // Renames the given desktop shortcut and informs the shell of this change.
 bool RenameDesktopShortcut(const base::FilePath& old_shortcut_path,
@@ -336,8 +361,12 @@ bool RenameDesktopShortcut(const base::FilePath& old_shortcut_path,
 // Renames an existing Chrome desktop profile shortcut. Must be called on the
 // FILE thread.
 void RenameChromeDesktopShortcutForProfile(
-    const base::string16& old_shortcut_filename,
-    const base::string16& new_shortcut_filename) {
+    const base::string16& old_profile_name,
+    const base::string16& new_profile_name,
+    std::set<base::FilePath>* profile_shortcuts,
+    std::set<base::FilePath>* desktop_contents) {
+  DCHECK(profile_shortcuts);
+  DCHECK(desktop_contents);
   DCHECK_CURRENTLY_ON(BrowserThread::FILE);
 
   base::FilePath user_shortcuts_directory;
@@ -347,32 +376,70 @@ void RenameChromeDesktopShortcutForProfile(
     return;
   }
 
-  const base::FilePath old_shortcut_path =
-      user_shortcuts_directory.Append(old_shortcut_filename);
+  BrowserDistribution* distribution = BrowserDistribution::GetDistribution();
+
+  // Get a new unique shortcut name.
+  const auto new_shortcut_filename =
+      profiles::internal::GetShortcutUniqueFilenameForProfile(
+          new_profile_name, distribution, *desktop_contents);
   const base::FilePath new_shortcut_path =
       user_shortcuts_directory.Append(new_shortcut_filename);
 
-  if (base::PathExists(old_shortcut_path)) {
+  if (!profile_shortcuts->empty()) {
+    // From all profile_shortcuts choose only with known (canonical) name.
+    profiles::internal::ShortcutFilenameMatcher matcher(old_profile_name,
+                                                        distribution);
+    auto it = std::find_if(profile_shortcuts->begin(), profile_shortcuts->end(),
+                           [&matcher](const base::FilePath& p) {
+                             return matcher.IsCanonical(p.BaseName().value());
+                           });
+    // If all profile_shortcuts were renamed by user, respect it and do not
+    // rename.
+    if (it == profile_shortcuts->end())
+      return;
+    const base::FilePath old_shortcut_path = *it;
+
     // Rename the old shortcut unless a system-level shortcut exists at the
     // destination, in which case the old shortcut is simply deleted.
     const base::FilePath possible_new_system_shortcut =
         system_shortcuts_directory.Append(new_shortcut_filename);
-    if (base::PathExists(possible_new_system_shortcut))
-      base::DeleteFile(old_shortcut_path, false);
-    else if (!RenameDesktopShortcut(old_shortcut_path, new_shortcut_path))
-      DLOG(ERROR) << "Could not rename Windows profile desktop shortcut.";
+    if (base::PathExists(possible_new_system_shortcut)) {
+      if (base::DeleteFile(old_shortcut_path, false)) {
+        profile_shortcuts->erase(old_shortcut_path);
+        desktop_contents->erase(old_shortcut_path);
+      } else {
+        DLOG(ERROR) << "Could not delete Windows profile desktop shortcut.";
+      }
+    } else {
+      if (RenameDesktopShortcut(old_shortcut_path, new_shortcut_path)) {
+        profile_shortcuts->erase(old_shortcut_path);
+        desktop_contents->erase(old_shortcut_path);
+        profile_shortcuts->insert(new_shortcut_path);
+        desktop_contents->insert(new_shortcut_path);
+      } else {
+        DLOG(ERROR) << "Could not rename Windows profile desktop shortcut.";
+      }
+    }
   } else {
-    // If the shortcut does not exist, it may have been renamed by the user. In
-    // that case, its name should not be changed.
+    // If the shortcut does not exist, it may have been deleted by the user.
     // It's also possible that a system-level shortcut exists instead - this
     // should only be the case for the original Chrome shortcut from an
     // installation. If that's the case, copy that one over - it will get its
     // properties updated by
     // |CreateOrUpdateDesktopShortcutsAndIconForProfile()|.
+    const auto old_shortcut_filename =
+        profiles::internal::GetShortcutFilenameForProfile(old_profile_name,
+                                                          distribution);
     const base::FilePath possible_old_system_shortcut =
         system_shortcuts_directory.Append(old_shortcut_filename);
-    if (base::PathExists(possible_old_system_shortcut))
-      base::CopyFile(possible_old_system_shortcut, new_shortcut_path);
+    if (base::PathExists(possible_old_system_shortcut)) {
+      if (base::CopyFile(possible_old_system_shortcut, new_shortcut_path)) {
+        profile_shortcuts->insert(new_shortcut_path);
+        desktop_contents->insert(new_shortcut_path);
+      } else {
+        DLOG(ERROR) << "Could not copy Windows profile desktop shortcut.";
+      }
+    }
   }
 }
 
@@ -428,24 +495,28 @@ void CreateOrUpdateDesktopShortcutsAndIconForProfile(
   // the following code may result in NOTREACHED() being hit.
   DCHECK(distribution->CanCreateDesktopShortcuts());
 
+  auto desktop_contents = ListUserDesktopContents([](...) { return true; });
+
+  const base::string16 command_line =
+      profiles::internal::CreateProfileShortcutFlags(params.profile_path);
+  ChromeCommandLineFilter filter(
+      chrome_exe, command_line,
+      params.action == ProfileShortcutManagerWin::UPDATE_NON_PROFILE_SHORTCUTS);
+
+  std::set<base::FilePath> shortcuts;
+  std::copy_if(desktop_contents.begin(), desktop_contents.end(),
+               std::inserter(shortcuts, shortcuts.begin()), filter);

[Alexei Svitkine (slow), 2016/01/05 19:55:22]

Why copy_if here instead of passing the filter directly to
ListUserDesktopContents()? If there's a good reason, it should at least have a
comment. But it's not obvious to me.

+
   if (params.old_profile_name != params.profile_name) {
-    const base::string16 old_shortcut_filename =
-        profiles::internal::GetShortcutFilenameForProfile(
-            params.old_profile_name,
-            distribution);
-    const base::string16 new_shortcut_filename =
-        profiles::internal::GetShortcutFilenameForProfile(params.profile_name,
-                                                          distribution);
-    RenameChromeDesktopShortcutForProfile(old_shortcut_filename,
-                                          new_shortcut_filename);
+    RenameChromeDesktopShortcutForProfile(params.old_profile_name,
+                                          params.profile_name, &shortcuts,
+                                          &desktop_contents);
   }
 
   ShellUtil::ShortcutProperties properties(ShellUtil::CURRENT_USER);
   installer::Product product(distribution);
   product.AddDefaultShortcutProperties(chrome_exe, &properties);
 
-  const base::string16 command_line =
-      profiles::internal::CreateProfileShortcutFlags(params.profile_path);
 
   // Only set the profile-specific properties when |profile_name| is non empty.
   // If it is empty, it means the shortcut being created should be a regular,
@@ -465,22 +536,17 @@ void CreateOrUpdateDesktopShortcutsAndIconForProfile(
   ShellUtil::ShortcutOperation operation =
       ShellUtil::SHELL_SHORTCUT_REPLACE_EXISTING;
 
-  std::vector<base::FilePath> shortcuts;
-  ListDesktopShortcutsWithCommandLine(chrome_exe, command_line,
-      params.action == ProfileShortcutManagerWin::UPDATE_NON_PROFILE_SHORTCUTS,
-      &shortcuts);
   if (params.create_mode == ProfileShortcutManagerWin::CREATE_WHEN_NONE_FOUND &&
       shortcuts.empty()) {
     const base::string16 shortcut_name =
-        profiles::internal::GetShortcutFilenameForProfile(params.profile_name,
-                                                          distribution);
-    shortcuts.push_back(base::FilePath(shortcut_name));
+        profiles::internal::GetShortcutUniqueFilenameForProfile(
+            params.profile_name, distribution, desktop_contents);
+    shortcuts.insert(base::FilePath(shortcut_name));
     operation = ShellUtil::SHELL_SHORTCUT_CREATE_IF_NO_SYSTEM_LEVEL;
   }
 
-  for (size_t i = 0; i < shortcuts.size(); ++i) {
-    const base::FilePath shortcut_name =
-        shortcuts[i].BaseName().RemoveExtension();
+  for (const auto& shortcut : shortcuts) {
+    const base::FilePath shortcut_name = shortcut.BaseName().RemoveExtension();
     properties.set_shortcut_name(shortcut_name.value());
     ShellUtil::CreateOrUpdateShortcut(ShellUtil::SHORTCUT_LOCATION_DESKTOP,
         distribution, properties, operation);
@@ -521,20 +587,18 @@ void DeleteDesktopShortcuts(const base::FilePath& profile_path,
 
   const base::string16 command_line =
       profiles::internal::CreateProfileShortcutFlags(profile_path);
-  std::vector<base::FilePath> shortcuts;
-  ListDesktopShortcutsWithCommandLine(chrome_exe, command_line, false,
-                                      &shortcuts);
+  ChromeCommandLineFilter filter(chrome_exe, command_line, false);
+  const auto shortcuts = ListUserDesktopContents(filter);

[anthonyvd, 2016/01/04 16:30:18]

nit: Can you write the actual return type when calling those
ListUserDesktopContents and GetShortcutUniqueFilenameForProfile? I think it's
relevant when reading the functions. Of course, you can leave auto in
range-based for loops and for iterators.

 
-  for (size_t i = 0; i < shortcuts.size(); ++i) {
+  for (const auto& shortcut : shortcuts) {
     // Use base::DeleteFile() instead of ShellUtil::RemoveShortcuts(), as the
     // latter causes non-profile taskbar shortcuts to be removed since it
     // doesn't consider the command-line of the shortcuts it deletes.
     // TODO(huangs): Refactor with ShellUtil::RemoveShortcuts().
-    base::win::UnpinShortcutFromTaskbar(shortcuts[i]);
-    base::DeleteFile(shortcuts[i], false);
+    base::win::UnpinShortcutFromTaskbar(shortcut);
+    base::DeleteFile(shortcut, false);
     // Notify the shell that the shortcut was deleted to ensure desktop refresh.
-    SHChangeNotify(SHCNE_DELETE, SHCNF_PATH, shortcuts[i].value().c_str(),
-                   NULL);
+    SHChangeNotify(SHCNE_DELETE, SHCNF_PATH, shortcut.value().c_str(), nullptr);
   }
 
   // If |ensure_shortcuts_remain| is true and deleting this profile caused the
@@ -572,9 +636,8 @@ bool HasAnyProfileShortcuts(const base::FilePath& profile_path) {
 
   const base::string16 command_line =
       profiles::internal::CreateProfileShortcutFlags(profile_path);
-  std::vector<base::FilePath> shortcuts;
-  ListDesktopShortcutsWithCommandLine(chrome_exe, command_line, false,
-                                      &shortcuts);
+  ChromeCommandLineFilter filter(chrome_exe, command_line, false);
+  const auto shortcuts = ListUserDesktopContents(filter);
   return !shortcuts.empty();
 }
 
@@ -643,6 +706,58 @@ base::string16 GetShortcutFilenameForProfile(
   return shortcut_name + installer::kLnkExt;
 }
 
+base::string16 GetShortcutUniqueFilenameForProfile(
+    const base::string16& profile_name,
+    BrowserDistribution* distribution,
+    const std::set<base::FilePath>& excludes) {
+  std::set<base::string16> excludes_names;
+  std::transform(excludes.begin(), excludes.end(),
+                 std::inserter(excludes_names, excludes_names.begin()),
+                 [](const base::FilePath& e) { return e.BaseName().value(); });
+
+  const auto base_name =
+      GetShortcutFilenameForProfile(profile_name, distribution);
+  auto name = base_name;
+  for (int uniquifier = 2; excludes_names.count(name) > 0; ++uniquifier)

[anthonyvd, 2016/01/04 16:30:18]

Little bikeshedding: I think this should start at "(1)" since that's seems more
consistent with the Chrome Download manager (which goes "download.ext",
"download (1).ext", etc) and Windows itself (which goes "file.ext", "file -
Copy.ext", "file - Copy (2).ext", etc). In both those cases, the original file
is index 0.

+    name = base::FilePath(base_name)
+        .InsertBeforeExtensionASCII(base::StringPrintf(" (%d)", uniquifier))
+        .value();
+  return name;
+}
+
+// Corresponds to GetShortcutUniqueFilenameForProfile.
+ShortcutFilenameMatcher::ShortcutFilenameMatcher(
+    const base::string16& profile_name,
+    BrowserDistribution* distribution)
+    : profile_shortcut_filename_(
+          GetShortcutFilenameForProfile(profile_name, distribution)),
+      lnk_ext_(installer::kLnkExt),
+      profile_shortcut_name_(profile_shortcut_filename_) {
+  DCHECK(profile_shortcut_name_.ends_with(lnk_ext_));
+  profile_shortcut_name_.remove_suffix(lnk_ext_.size());
+}
+
+bool ShortcutFilenameMatcher::IsCanonical(
+    const base::string16& filename) const {
+  if (filename == profile_shortcut_filename_)
+    return true;
+
+  base::StringPiece16 shortcut_suffix(filename);
+  if (!shortcut_suffix.starts_with(profile_shortcut_name_))
+    return false;
+  shortcut_suffix.remove_prefix(profile_shortcut_name_.size());
+
+  if (!shortcut_suffix.ends_with(lnk_ext_))
+    return false;
+  shortcut_suffix.remove_suffix(lnk_ext_.size());
+
+  if (shortcut_suffix.size() < 4 || !shortcut_suffix.starts_with(L" (") ||
+      !shortcut_suffix.ends_with(L")"))

[Alexei Svitkine (slow), 2016/01/05 19:55:22]

Nit: {}'s

+    return false;
+  return std::all_of(shortcut_suffix.begin() + 2, shortcut_suffix.end() - 1,
+                     [](wchar_t c) { return c >= L'0' && c <= L'9'; });

[Alexei Svitkine (slow), 2016/01/05 19:55:22]

Can this be isdigit?

+}
+
 base::string16 CreateProfileShortcutFlags(const base::FilePath& profile_path) {
   return base::StringPrintf(L"--%ls=\"%ls\"",
                             base::ASCIIToUTF16(