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

chrome/browser/profiles/profile_shortcut_manager_win.cc

 // Copyright (c) 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/profiles/profile_shortcut_manager_win.h"
 
 #include <string>
 #include <vector>
 
 #include "base/bind.h"
@@ skipping 167 matching lines @@
     if ((shortcut_command_line.empty() && include_empty_command_lines) ||
         (shortcut_command_line.find(command_line) != string16::npos)) {
       paths->push_back(path);
     }
   }
 }
 
 // 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.";
 }
 
 // Updates all desktop shortcuts for the given profile to have the specified
 // parameters. If |create_mode| is CREATE_WHEN_NONE_FOUND, a new shortcut is
 // created if no existing ones were found. Whether non-profile shortcuts should
 // 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,
     ProfileShortcutManagerWin::NonProfileShortcutAction action) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
   FilePath chrome_exe;
   if (!PathService::Get(base::FILE_EXE, &chrome_exe)) {
     NOTREACHED();
     return;
   }
 
   BrowserDistribution* distribution = BrowserDistribution::GetDistribution();
   // 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 =
       profiles::internal::CreateProfileShortcutFlags(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,
   // non-profile Chrome shortcut.
   if (!profile_name.empty()) {
     const FilePath shortcut_icon =
@@ skipping 13 matching lines @@
   std::vector<FilePath> shortcuts;
   ListDesktopShortcutsWithCommandLine(chrome_exe, command_line,
       action == ProfileShortcutManagerWin::UPDATE_NON_PROFILE_SHORTCUTS,
       &shortcuts);
   if (create_mode == ProfileShortcutManagerWin::CREATE_WHEN_NONE_FOUND &&
       shortcuts.empty()) {
     const string16 shortcut_name =
         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) {
     const FilePath shortcut_name = shortcuts[i].BaseName().RemoveExtension();
     properties.set_shortcut_name(shortcut_name.value());
     ShellUtil::CreateOrUpdateShortcut(ShellUtil::SHORTCUT_LOCATION_DESKTOP,
         distribution, properties, operation);
   }
 }
 
@@ skipping 57 matching lines @@
     // 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);
 
     ShellUtil::ShortcutProperties properties(ShellUtil::CURRENT_USER);
     product.AddDefaultShortcutProperties(chrome_exe, &properties);
     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);
   }
 }
 
 // Returns true if profile at |profile_path| has any shortcuts. Does not
 // consider non-profile shortcuts. Must be called on the FILE thread.
 bool HasAnyProfileShortcuts(const FilePath& profile_path) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
   FilePath chrome_exe;
   if (!PathService::Get(base::FILE_EXE, &chrome_exe)) {
@@ skipping 142 matching lines @@
   CreateOrUpdateShortcutsForProfileAtPath(profile_path, UPDATE_EXISTING_ONLY,
                                           IGNORE_NON_PROFILE_SHORTCUTS);
 }
 
 void ProfileShortcutManagerWin::OnProfileAvatarChanged(
     const FilePath& profile_path) {
   CreateOrUpdateShortcutsForProfileAtPath(profile_path, UPDATE_EXISTING_ONLY,
                                           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();
   DCHECK_EQ(2U, cache.GetNumberOfProfiles());
   // Get the index of the current profile, in order to find the index of the
   // other profile.
   size_t current_profile_index = cache.GetIndexOfProfileWithPath(profile_path);
   size_t other_profile_index = (current_profile_index == 0) ? 1 : 0;
   return cache.GetPathOfProfileAtIndex(other_profile_index);
 }
 
 void ProfileShortcutManagerWin::CreateOrUpdateShortcutsForProfileAtPath(
     const FilePath& profile_path,
     CreateOrUpdateMode create_mode,
     NonProfileShortcutAction action) {
   ProfileInfoCache* cache = &profile_manager_->GetProfileInfoCache();
   size_t profile_index = cache->GetIndexOfProfileWithPath(profile_path);
   if (profile_index == std::string::npos)
     return;
   bool remove_badging = cache->GetNumberOfProfiles() == 1;
 
   string16 old_shortcut_appended_name =
       cache->GetShortcutNameOfProfileAtIndex(profile_index);
 
   string16 new_shortcut_appended_name;
   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 =
         cache->GetAvatarIconIndexOfProfileAtIndex(profile_index);
     gfx::Image profile_avatar_image = ResourceBundle::GetSharedInstance().
         GetNativeImageNamed(
             cache->GetDefaultAvatarIconResourceIDAtIndex(profile_icon_index));
 
     DCHECK(!profile_avatar_image.IsEmpty());
     const SkBitmap* profile_avatar_bitmap = profile_avatar_image.ToSkBitmap();
     // Make a copy of the SkBitmap to ensure that we can safely use the image
     // data on the FILE thread.
     profile_avatar_bitmap->deepCopyTo(&profile_avatar_bitmap_copy,
                                       profile_avatar_bitmap->getConfig());
   }
   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);
 }