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 95 matching lines @@
   const FilePath icon_path =
       profile_path.AppendASCII(profiles::internal::kProfileIconFileName);
   // TODO(asvitkine): Create icon with a large 256x256 bitmap.
   if (!IconUtil::CreateIconFileFromSkBitmap(final_bitmap, SkBitmap(),
                                             icon_path))
     return FilePath();
 
   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
 // like "foo~2" to their full names.
 FilePath ConvertToLongPath(const FilePath& path) {
   const size_t length = GetLongPathName(path.value().c_str(), NULL, 0);
   if (length != 0 && length != path.value().length()) {
     std::vector<wchar_t> long_path(length);
     if (GetLongPathName(path.value().c_str(), &long_path[0], length) != 0)
       return FilePath(&long_path[0]);
@@ skipping 20 matching lines @@
 }
 
 // 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 FilePath& chrome_exe,
                                          const string16& command_line,
                                          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,
       file_util::FileEnumerator::FILES);
   for (FilePath path = enumerator.Next(); !path.empty();
        path = enumerator.Next()) {
     string16 shortcut_command_line;
     if (!IsChromeShortcut(path, chrome_exe, &shortcut_command_line))
       continue;
 
     // 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) != 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;
-  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
 // 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);
   }
 }
 
 // Returns true if any desktop shortcuts exist with target |chrome_exe|,
 // 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,
       file_util::FileEnumerator::FILES);
   for (FilePath path = enumerator.Next(); !path.empty();
        path = enumerator.Next()) {
     if (IsChromeShortcut(path, chrome_exe, NULL))
       return true;
   }
 
@@ skipping 42 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);
 }