ProfileShortcutManagerWin: Don't create user level shortcut when system level one exists.

chrome/browser/profiles/profile_shortcut_manager_win.cc

Index: chrome/browser/profiles/profile_shortcut_manager_win.cc
===================================================================
--- chrome/browser/profiles/profile_shortcut_manager_win.cc	(revision 176381)
+++ chrome/browser/profiles/profile_shortcut_manager_win.cc	(working copy)
@@ -113,14 +113,25 @@
   return icon_path;
 }
 
-// Gets the directory where to create desktop shortcuts.
-bool GetDesktopShortcutsDirectory(FilePath* directory) {
-  const bool result =
-      ShellUtil::GetShortcutPath(ShellUtil::SHORTCUT_LOCATION_DESKTOP,
-                                 BrowserDistribution::GetDistribution(),
-                                 ShellUtil::CURRENT_USER, directory);
-  DCHECK(result);
-  return result;
+// Gets the user and system directories for desktop shortcuts.

[gab, 2013/01/16 17:44:00]

nit: Add comment about return value?

[Alexei Svitkine (slow), 2013/01/16 17:59:56]

Done.

+bool GetDesktopShortcutsDirectories(FilePath* user_shortcuts_directory,
+                                    FilePath* system_shortcuts_directory) {
+  BrowserDistribution* distribution = BrowserDistribution::GetDistribution();
+  if (user_shortcuts_directory &&
+      !ShellUtil::GetShortcutPath(ShellUtil::SHORTCUT_LOCATION_DESKTOP,
+                                  distribution, ShellUtil::CURRENT_USER,
+                                  user_shortcuts_directory)) {
+    NOTREACHED();
+    return false;
+  }
+  if (system_shortcuts_directory &&
+      !ShellUtil::GetShortcutPath(ShellUtil::SHORTCUT_LOCATION_DESKTOP,
+                                  distribution, ShellUtil::SYSTEM_LEVEL,
+                                  system_shortcuts_directory)) {
+    NOTREACHED();
+    return false;
+  }
+  return true;
 }
 
 // Returns the long form of |path|, which will expand any shortened components
@@ -161,7 +172,7 @@
                                          bool include_empty_command_lines,
                                          std::vector<FilePath>* paths) {
   FilePath shortcuts_directory;
-  if (!GetDesktopShortcutsDirectory(&shortcuts_directory))
+  if (!GetDesktopShortcutsDirectories(&shortcuts_directory, NULL))

[gab, 2013/01/16 17:44:00]

nit: perhaps rename all instances of |shortcuts_directory| to
|user_shortcuts_directory| even when |system_shortcuts_directory| is not used. I
think this makes the callsite more readable (i.e. otherwise the reader can't
assume anything and has to go lookup GetDesktopShortcutsDirectories's
definition).

[Alexei Svitkine (slow), 2013/01/16 17:59:56]

Done.

     return;
 
   file_util::FileEnumerator enumerator(shortcuts_directory, false,
@@ -185,23 +196,43 @@
 // Renames an existing Chrome desktop profile shortcut. Must be called on the
 // FILE thread.
 void RenameChromeDesktopShortcutForProfile(
-    const string16& old_shortcut_file,
-    const string16& new_shortcut_file) {
+    const string16& old_shortcut_filename,
+    const string16& new_shortcut_filename) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
-  FilePath shortcuts_directory;
-  if (!GetDesktopShortcutsDirectory(&shortcuts_directory))
+  FilePath user_shortcuts_directory;
+  FilePath system_shortcuts_directory;
+  if (!GetDesktopShortcutsDirectories(&user_shortcuts_directory,
+                                      &system_shortcuts_directory)) {
     return;
+  }
 
-  FilePath old_shortcut_path = shortcuts_directory.Append(old_shortcut_file);
-  // If the shortcut does not exist, it may have been renamed by the user. In
-  // that case, its name should not be changed.
-  if (!file_util::PathExists(old_shortcut_path))
-    return;
+  const FilePath old_shortcut_path =
+      user_shortcuts_directory.Append(old_shortcut_filename);
+  const FilePath new_shortcut_path =
+      user_shortcuts_directory.Append(new_shortcut_filename);
 
-  FilePath new_shortcut_path = shortcuts_directory.Append(new_shortcut_file);
-  if (!file_util::Move(old_shortcut_path, new_shortcut_path))
-    LOG(ERROR) << "Could not rename Windows profile desktop shortcut.";
+  if (file_util::PathExists(old_shortcut_path)) {
+    // Rename the old shortcut unless a system-level shortcut exists at the
+    // destination, in which case the old shortcut is simply deleted.
+    const FilePath possible_new_system_shortcut =
+        system_shortcuts_directory.Append(new_shortcut_filename);
+    if (file_util::PathExists(possible_new_system_shortcut))
+      file_util::Delete(old_shortcut_path, false);
+    else if (!file_util::Move(old_shortcut_path, new_shortcut_path))
+      LOG(ERROR) << "Could not rename Windows profile desktop shortcut.";

[gab, 2013/01/16 17:44:00]

nit: DLOG(ERROR) --> We will never actually get release logs from users and this
otherwise only bloats the binary with an unnecessary string imo.

[Alexei Svitkine (slow), 2013/01/16 17:59:56]

Done.

+  } else {
+    // If the shortcut does not exist, it may have been renamed by the user. In
+    // that case, its name should not be changed.
+    // It's also possible that a system-level shortcut exists instead, for
+    // example the original Chrome shortcut from an installation. If that's the
+    // case, copy that one over - it will get its properties updated by the code
+    // in |CreateOrUpdateDesktopShortcutsForProfile()|.
+    const FilePath possible_old_system_shortcut =
+        system_shortcuts_directory.Append(old_shortcut_filename);
+    if (file_util::PathExists(possible_old_system_shortcut))
+      file_util::CopyFile(possible_old_system_shortcut, new_shortcut_path);

[gab, 2013/01/16 17:44:00]

Actually I just realized: if the shortcut has been renamed and there is a
system-level shortcut this will duplicate this profile's shortcut (i.e. one with
the custom name and one with the default name)...

Do we have a better way to find manually renamed shortcuts for this profile?

[gab, 2013/01/16 17:44:52]

Should probably add a test for this too... shortcuts are fun :)!

[Alexei Svitkine (slow), 2013/01/16 17:59:56]

There shouldn't ever be a system-level *profile* shortcut (unless a user does
something peculiar like manually copying a user level profile shortcut to the
system-level desktop directory). So I don't think this is an issue in practice.

This code deals primarily with the case where the "old shortcut" is a
non-profile shortcut.

[gab, 2013/01/16 18:17:32]

Ah right :) -- perhaps the comment could be clarified to highlight this.

[Alexei Svitkine (slow), 2013/01/16 18:40:42]

Comment updated.

[gab, 2013/01/16 18:42:41]

Thanks, still lgtm.

+  }
 }
 
 // Updates all desktop shortcuts for the given profile to have the specified
@@ -210,6 +241,7 @@
 // be updated is specified by |action|. Must be called on the FILE thread.
 void CreateOrUpdateDesktopShortcutsForProfile(
     const FilePath& profile_path,
+    const string16& old_profile_name,
     const string16& profile_name,
     const SkBitmap& avatar_image,
     ProfileShortcutManagerWin::CreateOrUpdateMode create_mode,
@@ -226,9 +258,20 @@
   // Ensure that the distribution supports creating shortcuts. If it doesn't,
   // the following code may result in NOTREACHED() being hit.
   DCHECK(distribution->CanCreateDesktopShortcuts());
-  installer::Product product(distribution);
 
+  if (old_profile_name != profile_name) {
+    const string16 old_shortcut_filename =
+        profiles::internal::GetShortcutFilenameForProfile(old_profile_name,
+                                                          distribution);
+    const string16 new_shortcut_filename =
+        profiles::internal::GetShortcutFilenameForProfile(profile_name,
+                                                          distribution);
+    RenameChromeDesktopShortcutForProfile(old_shortcut_filename,
+                                          new_shortcut_filename);
+  }
+
   ShellUtil::ShortcutProperties properties(ShellUtil::CURRENT_USER);
+  installer::Product product(distribution);
   product.AddDefaultShortcutProperties(chrome_exe, &properties);
 
   const string16 command_line =
@@ -262,7 +305,7 @@
         profiles::internal::GetShortcutFilenameForProfile(profile_name,
                                                           distribution);
     shortcuts.push_back(FilePath(shortcut_name));
-    operation = ShellUtil::SHELL_SHORTCUT_CREATE_ALWAYS;
+    operation = ShellUtil::SHELL_SHORTCUT_CREATE_IF_NO_SYSTEM_LEVEL;
   }
 
   for (size_t i = 0; i < shortcuts.size(); ++i) {
@@ -277,7 +320,7 @@
 // regardless of their command line arguments.
 bool ChromeDesktopShortcutsExist(const FilePath& chrome_exe) {
   FilePath shortcuts_directory;
-  if (!GetDesktopShortcutsDirectory(&shortcuts_directory))
+  if (!GetDesktopShortcutsDirectories(&shortcuts_directory, NULL))
     return false;
 
   file_util::FileEnumerator enumerator(shortcuts_directory, false,
@@ -340,9 +383,9 @@
     properties.set_shortcut_name(
         profiles::internal::GetShortcutFilenameForProfile(string16(),
                                                           distribution));
-    ShellUtil::CreateOrUpdateShortcut(ShellUtil::SHORTCUT_LOCATION_DESKTOP,
-                                      distribution, properties,
-                                      ShellUtil::SHELL_SHORTCUT_CREATE_ALWAYS);
+    ShellUtil::CreateOrUpdateShortcut(
+        ShellUtil::SHORTCUT_LOCATION_DESKTOP, distribution, properties,
+        ShellUtil::SHELL_SHORTCUT_CREATE_IF_NO_SYSTEM_LEVEL);
   }
 }
 
@@ -505,32 +548,6 @@
                                           IGNORE_NON_PROFILE_SHORTCUTS);
 }
 
-void ProfileShortcutManagerWin::StartProfileShortcutNameChange(
-    const FilePath& profile_path,
-    const string16& old_profile_name) {
-  const ProfileInfoCache& cache = profile_manager_->GetProfileInfoCache();
-  size_t profile_index = cache.GetIndexOfProfileWithPath(profile_path);
-  if (profile_index == std::string::npos)
-    return;
-  // If the shortcut will have an appended name, get the profile name.
-  string16 new_profile_name;
-  if (cache.GetNumberOfProfiles() != 1)
-    new_profile_name = cache.GetNameOfProfileAtIndex(profile_index);
-
-  BrowserDistribution* distribution = BrowserDistribution::GetDistribution();
-  const string16 old_shortcut_file =
-      profiles::internal::GetShortcutFilenameForProfile(old_profile_name,
-                                                        distribution);
-  const string16 new_shortcut_file =
-      profiles::internal::GetShortcutFilenameForProfile(new_profile_name,
-                                                        distribution);
-  BrowserThread::PostTask(
-      BrowserThread::FILE, FROM_HERE,
-      base::Bind(&RenameChromeDesktopShortcutForProfile,
-                 old_shortcut_file,
-                 new_shortcut_file));
-}
-
 FilePath ProfileShortcutManagerWin::GetOtherProfilePath(
     const FilePath& profile_path) {
   const ProfileInfoCache& cache = profile_manager_->GetProfileInfoCache();
@@ -559,12 +576,6 @@
   if (!remove_badging)
     new_shortcut_appended_name = cache->GetNameOfProfileAtIndex(profile_index);
 
-  if (create_mode == UPDATE_EXISTING_ONLY &&
-      new_shortcut_appended_name != old_shortcut_appended_name) {
-    // TODO(asvitkine): Fold this into |UpdateDesktopShortcutsForProfile()|.
-    StartProfileShortcutNameChange(profile_path, old_shortcut_appended_name);
-  }
-
   SkBitmap profile_avatar_bitmap_copy;
   if (!remove_badging) {
     size_t profile_icon_index =
@@ -583,8 +594,9 @@
   BrowserThread::PostTask(
       BrowserThread::FILE, FROM_HERE,
       base::Bind(&CreateOrUpdateDesktopShortcutsForProfile,
-                 profile_path, new_shortcut_appended_name,
-                 profile_avatar_bitmap_copy, create_mode, action));
+                 profile_path, old_shortcut_appended_name,
+                 new_shortcut_appended_name, profile_avatar_bitmap_copy,
+                 create_mode, action));
 
   cache->SetShortcutNameOfProfileAtIndex(profile_index,
                                          new_shortcut_appended_name);