[Fixit-Dec-2012] Keep old shortcut's arguments when replacing an existing shortcut and not specifying any arguments for the new shortcut.

R=robertshield@chromium.org
BUG=146471
TEST=base_unittests --gtest_filter=ShortcutTest.* (hard to test locally as an over-install of the same version triggers a repair install which uses SHORTCUT_CREATE_ALWAYS -- I'll setup an old dev-channel and wait for it to autoupdate to the new one once this lands...).


Review URL: https://chromiumcodereview.appspot.com/11740014

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@175008 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 48 insertions, 13 deletions

diff --git a/base/win/shortcut.cc b/base/win/shortcut.cc
index 912bdb5..eb7a72c 100644
--- a/base/win/shortcut.cc
+++ b/base/win/shortcut.cc
@@ -55,6 +55,11 @@ bool CreateOrUpdateShortcutLink(const FilePath& shortcut_path,
 
   bool shortcut_existed = file_util::PathExists(shortcut_path);
 
+  // Interfaces to the old shortcut when replacing an existing shortcut.
+  ScopedComPtr<IShellLink> old_i_shell_link;
+  ScopedComPtr<IPersistFile> old_i_persist_file;
+
+  // Interfaces to the shortcut being created/updated.
   ScopedComPtr<IShellLink> i_shell_link;
   ScopedComPtr<IPersistFile> i_persist_file;
   switch (operation) {
@@ -66,13 +71,13 @@ bool CreateOrUpdateShortcutLink(const FilePath& shortcut_path,
                                    &i_persist_file);
       break;
     case SHORTCUT_REPLACE_EXISTING:
-      InitializeShortcutInterfaces(shortcut_path.value().c_str(), &i_shell_link,
-                                   &i_persist_file);
+      InitializeShortcutInterfaces(shortcut_path.value().c_str(),
+                                   &old_i_shell_link, &old_i_persist_file);
       // Confirm |shortcut_path| exists and is a shortcut by verifying
-      // |i_persist_file| was successfully initialized in the call above. If so,
-      // re-initialize the interfaces to begin writing a new shortcut (to
+      // |old_i_persist_file| was successfully initialized in the call above. If
+      // so, initialize the interfaces to begin writing a new shortcut (to
       // overwrite the current one if successful).
-      if (i_persist_file.get())
+      if (old_i_persist_file.get())
         InitializeShortcutInterfaces(NULL, &i_shell_link, &i_persist_file);
       break;
     default:
@@ -94,9 +99,15 @@ bool CreateOrUpdateShortcutLink(const FilePath& shortcut_path,
     return false;
   }
 
-  if ((properties.options & ShortcutProperties::PROPERTIES_ARGUMENTS) &&
-      FAILED(i_shell_link->SetArguments(properties.arguments.c_str()))) {
-    return false;
+  if (properties.options & ShortcutProperties::PROPERTIES_ARGUMENTS) {
+    if (FAILED(i_shell_link->SetArguments(properties.arguments.c_str())))
+      return false;
+  } else if (old_i_persist_file.get()) {
+    wchar_t current_arguments[MAX_PATH] = {0};
+    if (SUCCEEDED(old_i_shell_link->GetArguments(current_arguments,
+                                                 MAX_PATH))) {
+      i_shell_link->SetArguments(current_arguments);
+    }
   }
 
   if ((properties.options & ShortcutProperties::PROPERTIES_DESCRIPTION) &&
@@ -132,6 +143,11 @@ bool CreateOrUpdateShortcutLink(const FilePath& shortcut_path,
     }
   }
 
+  // Release the interfaces to the old shortcut to make sure it doesn't prevent
+  // overwriting it if needed.
+  old_i_persist_file.Release();
+  old_i_shell_link.Release();
+
   HRESULT result = i_persist_file->Save(shortcut_path.value().c_str(), TRUE);
 
   // Release the interfaces in case the SHChangeNotify call below depends on
diff --git a/base/win/shortcut.h b/base/win/shortcut.h
index c1e7d5c..52ac1a7 100644
--- a/base/win/shortcut.h
+++ b/base/win/shortcut.h
@@ -18,6 +18,8 @@ enum ShortcutOperation {
   // Create a new shortcut (overwriting if necessary).
   SHORTCUT_CREATE_ALWAYS = 0,
   // Overwrite an existing shortcut (fails if the shortcut doesn't exist).
+  // If the arguments are not specified on the new shortcut, keep the old
+  // shortcut's arguments.
   SHORTCUT_REPLACE_EXISTING,
   // Update specified properties only on an existing shortcut.
   SHORTCUT_UPDATE_EXISTING,
diff --git a/base/win/shortcut_unittest.cc b/base/win/shortcut_unittest.cc
index 26b22be..70cc852 100644
--- a/base/win/shortcut_unittest.cc
+++ b/base/win/shortcut_unittest.cc
@@ -212,7 +212,7 @@ TEST_F(ShortcutTest, FailUpdateShortcutThatDoesNotExist) {
   ASSERT_FALSE(file_util::PathExists(link_file_));
 }
 
-TEST_F(ShortcutTest, TruncateShortcutAllProperties) {
+TEST_F(ShortcutTest, ReplaceShortcutAllProperties) {
   ASSERT_TRUE(base::win::CreateOrUpdateShortcutLink(
       link_file_, link_properties_, base::win::SHORTCUT_CREATE_ALWAYS));
 
@@ -222,29 +222,46 @@ TEST_F(ShortcutTest, TruncateShortcutAllProperties) {
   base::win::ValidateShortcut(link_file_, link_properties_2_);
 }
 
-TEST_F(ShortcutTest, TruncateShortcutSomeProperties) {
+TEST_F(ShortcutTest, ReplaceShortcutSomeProperties) {
   ASSERT_TRUE(base::win::CreateOrUpdateShortcutLink(
       link_file_, link_properties_, base::win::SHORTCUT_CREATE_ALWAYS));
 
   base::win::ShortcutProperties new_properties;
   new_properties.set_target(link_properties_2_.target);
+  new_properties.set_arguments(link_properties_2_.arguments);
   new_properties.set_description(link_properties_2_.description);
   ASSERT_TRUE(base::win::CreateOrUpdateShortcutLink(
       link_file_, new_properties, base::win::SHORTCUT_REPLACE_EXISTING));
 
   // Expect only properties in |new_properties| to be set, all other properties
   // should have been overwritten.
-  base::win::ShortcutProperties expected_properties = new_properties;
+  base::win::ShortcutProperties expected_properties(new_properties);
   expected_properties.set_working_dir(FilePath());
-  expected_properties.set_arguments(string16());
   expected_properties.set_icon(FilePath(), 0);
   expected_properties.set_app_id(string16());
   expected_properties.set_dual_mode(false);
   base::win::ValidateShortcut(link_file_, expected_properties);
 }
 
-TEST_F(ShortcutTest, FailTruncateShortcutThatDoesNotExist) {
+TEST_F(ShortcutTest, FailReplaceShortcutThatDoesNotExist) {
   ASSERT_FALSE(base::win::CreateOrUpdateShortcutLink(
       link_file_, link_properties_, base::win::SHORTCUT_REPLACE_EXISTING));
   ASSERT_FALSE(file_util::PathExists(link_file_));
 }
+
+// Test that the old arguments remain on the replaced shortcut when not
+// otherwise specified.
+TEST_F(ShortcutTest, ReplaceShortcutKeepOldArguments) {
+  ASSERT_TRUE(base::win::CreateOrUpdateShortcutLink(
+      link_file_, link_properties_, base::win::SHORTCUT_CREATE_ALWAYS));
+
+  // Do not explicitly set the arguments.
+  link_properties_2_.options &=
+      ~base::win::ShortcutProperties::PROPERTIES_ARGUMENTS;
+  ASSERT_TRUE(base::win::CreateOrUpdateShortcutLink(
+      link_file_, link_properties_2_, base::win::SHORTCUT_REPLACE_EXISTING));
+
+  base::win::ShortcutProperties expected_properties(link_properties_2_);
+  expected_properties.set_arguments(link_properties_.arguments);
+  base::win::ValidateShortcut(link_file_, expected_properties);
+}