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)
@@ -185,21 +185,51 @@
 // 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;

[gab, 2013/01/15 23:09:14]

Rename this to user_shortcuts_directory since you now have
system_shortcuts_directory.

[Alexei Svitkine (slow), 2013/01/16 16:11:44]

Done.

   if (!GetDesktopShortcutsDirectory(&shortcuts_directory))

[gab, 2013/01/15 23:09:14]

I don't really see the value of this method, an if(!..){NOTREACHED;return;}
block like below seems just fine to me... Furthermore this call recomputes the
BrowserDistribution when it could easily be re-used here.

[Alexei Svitkine (slow), 2013/01/15 23:34:43]

The method is called from other functions too, to make the code more readable.
They would all have to be changed to the more verbose versions.

Maybe we can change it to:

GetDesktopShortcutsDirectories(FilePath* user_shortcuts_directory, FilePath*
system_shortcuts_directory);

And have the params be optional (if passed in NULL, then it won't fill them in).
This way, it could re-use the distribution for both calls and still be very
concise from the call sites. WDYT?

[Alexei Svitkine (slow), 2013/01/16 16:11:44]

I've now done this in the latest patchset.

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

sgtm.

     return;
 
-  FilePath old_shortcut_path = shortcuts_directory.Append(old_shortcut_file);
+  FilePath system_shortcuts_directory;
+  if (!ShellUtil::GetShortcutPath(ShellUtil::SHORTCUT_LOCATION_DESKTOP,
+                                  BrowserDistribution::GetDistribution(),
+                                  ShellUtil::SYSTEM_LEVEL,
+                                  &system_shortcuts_directory)) {
+    NOTREACHED();
+    return;
+  }
+
+  const FilePath old_shortcut_path =
+      shortcuts_directory.Append(old_shortcut_filename);
+  const FilePath new_shortcut_path =
+      shortcuts_directory.Append(new_shortcut_filename);
+
   // 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))
+  if (!file_util::PathExists(old_shortcut_path)) {
+    // It's 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);
     return;
+  }
 
-  FilePath new_shortcut_path = shortcuts_directory.Append(new_shortcut_file);
+  // If a system level shortcut exists at the destination, then simply delete
+  // the old shortcut.
+  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);
+    return;

[gab, 2013/01/15 23:09:14]

I'm not a fan of the multiple early returns I feel an if{}else if{}else if{}
block would be more readable here (as it is now it feels like continuous code at
first glance, but it's really not as any of the conditions met along the way
cause an early return).

What do you think of replacing 209-234 with something like:

  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.";
  } 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);
  }

[Alexei Svitkine (slow), 2013/01/16 16:11:44]

I like your suggested flow better, done!

+  }
+
   if (!file_util::Move(old_shortcut_path, new_shortcut_path))
     LOG(ERROR) << "Could not rename Windows profile desktop shortcut.";
 }
@@ -210,6 +240,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 +257,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 +304,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) {
@@ -340,9 +382,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 +547,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 +575,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 +593,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);