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 168 matching 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) {

[gab, 2013/01/14 21:49:30]

optional nit: Rename these parameters to old_shortcut_filename and
new_shortcut_filename... from the code they look like filenames, not the file
itself.

[Alexei Svitkine (slow), 2013/01/15 22:03:02]

Done.

[sail, 2013/01/15 22:12:15]

Didn't see this change. I'm guessing you're doing this in a separate CL.

[Alexei Svitkine (slow), 2013/01/15 22:18:34]

It should be there, take a closer look at:
https://codereview.chromium.org/11876027/diff/18001/chrome/browser/profiles/p...

[sail, 2013/01/15 22:21:36]

Oops, I was thinking of the test member variable renaming. LGTM again!

   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
   FilePath shortcuts_directory;
   if (!GetDesktopShortcutsDirectory(&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;
 
+  // If a system shortcut exists at the destination, then simply delete the
+  // old shortcut.
+  FilePath system_shortcuts_directory;
+  DCHECK(ShellUtil::GetShortcutPath(ShellUtil::SHORTCUT_LOCATION_DESKTOP,

[gab, 2013/01/14 21:49:30]

Take the call out of the DCHECK, store the result and DCHECK on that.

Or instead do:
if (!ShellUtil::GetShortcutPath...)
  NOTREACHED();

Otherwise this code will not run in Release builds..

Also if this fails you probably don't want to keep going with the verification.

[Alexei Svitkine (slow), 2013/01/15 22:03:02]

Doh, thanks for catching this. Fixed.

+                                    BrowserDistribution::GetDistribution(),
+                                    ShellUtil::SYSTEM_LEVEL,
+                                    &system_shortcuts_directory));
+  const FilePath possible_system_shortcut =

[sail, 2013/01/14 21:45:19]

new_system_shortcut_path

[Alexei Svitkine (slow), 2013/01/15 22:03:02]

I've changed it to |possible_new_system_shortcut|, to still have the name
indicate that it may or may not exist.

+      system_shortcuts_directory.Append(new_shortcut_file);
+  if (file_util::PathExists(possible_system_shortcut)) {
+    file_util::Delete(old_shortcut_path, false);
+    return;
+  }
+
   FilePath new_shortcut_path = shortcuts_directory.Append(new_shortcut_file);

[sail, 2013/01/14 21:45:19]

new_user_shortcut_path

[Alexei Svitkine (slow), 2013/01/15 22:03:02]

All the paths here are assumed to be user paths except for
|possible_new_system_shortcut|, which is named to indicate that. I suppose I
could change things to be more explicit (e.g. rename old_shortcut_path to
old_user_shortcut_path), but it seems unnecessary to me. WDYT?

   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,
@@ skipping 42 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;

[gab, 2013/01/14 21:49:30]

This will prevent creation of all profile shortcuts when a system-level shortcut
is present, I think you want to only prevent creation of the shortcut when it is
unbadged (i.e. only 1 profile).

It is unfortunate that the global shortcut is still visible when you have 2
profiles, but I think you still want to show the both profile's badged/named
shortcut in that case as it feels otherwise confusing... I guess the global
shortcut will keep the current behavior of opening the last profile.

[Alexei Svitkine (slow), 2013/01/14 22:57:14]

Makes sense. Just to clarify, SHELL_SHORTCUT_CREATE_IF_NO_SYSTEM_LEVEL will
prevent the creation of a user-level shortcut "Foo.lnk" when there's a system
level shortcut "Google Chrome.lnk"? When I wrote this, I did it under the
impression that it would check for a system-level shortcut with the same name,
in which case this code should be correct, I think. Please clarify.

[gab, 2013/01/14 23:04:42]

Ah, you're right, I wrongly assumed it would prevent all creations of
user-level, but just looked at the details of the implementation and you're
right; this was not really intended when I wrote it, but I guess it has
clever/useful side-effects now :). Could you add to the documentation for
SHELL_SHORTCUT_CREATE_IF_NO_SYSTEM_LEVEL to be clear that this is the intended
behavior for it to stick?

Cheers!
Gab

[Alexei Svitkine (slow), 2013/01/15 22:03:02]

Done.

   }
 
   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 226 matching lines @@
   }
   BrowserThread::PostTask(
       BrowserThread::FILE, FROM_HERE,
       base::Bind(&CreateOrUpdateDesktopShortcutsForProfile,
                  profile_path, new_shortcut_appended_name,
                  profile_avatar_bitmap_copy, create_mode, action));
 
   cache->SetShortcutNameOfProfileAtIndex(profile_index,
                                          new_shortcut_appended_name);
 }