Installer: adding ResolveShortcutProperties(); updating shortcut icons during shortcut migration.

chrome/installer/util/shell_util_unittest.cc

Index: chrome/installer/util/shell_util_unittest.cc
diff --git a/chrome/installer/util/shell_util_unittest.cc b/chrome/installer/util/shell_util_unittest.cc
index 1d51b3ca731b15b90ee32684c57902e07999c15e..066801c1f4b9a76792aeb923c3f73667520a8bdd 100644
--- a/chrome/installer/util/shell_util_unittest.cc
+++ b/chrome/installer/util/shell_util_unittest.cc
@@ -27,6 +27,7 @@
 namespace {
 
 const wchar_t kManganeseExe[] = L"manganese.exe";
+const wchar_t kOtherIco[] = L"other.ico";
 
 // TODO(huangs): Separate this into generic shortcut tests and Chrome-specific
 // tests. Specifically, we should not overly rely on getting shortcut properties
@@ -47,6 +48,9 @@ class ShellUtilShortcutTest : public testing::Test {
     manganese_exe_ = temp_dir_.path().Append(kManganeseExe);
     EXPECT_EQ(0, file_util::WriteFile(manganese_exe_, "", 0));
 
+    other_ico_ = temp_dir_.path().Append(kOtherIco);
+    EXPECT_EQ(0, file_util::WriteFile(other_ico_, "", 0));
+
     ASSERT_TRUE(fake_user_desktop_.CreateUniqueTempDir());
     ASSERT_TRUE(fake_common_desktop_.CreateUniqueTempDir());
     ASSERT_TRUE(fake_user_quick_launch_.CreateUniqueTempDir());
@@ -82,11 +86,9 @@ class ShellUtilShortcutTest : public testing::Test {
     test_properties_.set_dual_mode(true);
   }
 
-  // Validates that the shortcut at |location| matches |properties| (and
-  // implicit default properties) for |dist|.
-  // Note: This method doesn't verify the |pin_to_taskbar| property as it
-  // implies real (non-mocked) state which is flaky to test.
-  void ValidateChromeShortcut(
+  // Returns the expected path of a test shortcut. Returns an empty FilePath on
+  // failure.
+  base::FilePath GetExpectedShortcutPath(
       ShellUtil::ShortcutLocation location,
       BrowserDistribution* dist,
       const ShellUtil::ShortcutProperties& properties) {
@@ -110,18 +112,26 @@ class ShellUtilShortcutTest : public testing::Test {
         break;
       default:
         ADD_FAILURE() << "Unknown location";
-        return;
+        return base::FilePath();
     }
 
-    string16 shortcut_name;
-    if (properties.has_shortcut_name()) {
-      shortcut_name = properties.shortcut_name;
-    } else {
-      shortcut_name =
-          dist_->GetShortcutName(BrowserDistribution::SHORTCUT_CHROME);
-    }
+    string16 shortcut_name = properties.has_shortcut_name() ?
+        properties.shortcut_name :
+        dist_->GetShortcutName(BrowserDistribution::SHORTCUT_CHROME);
     shortcut_name.append(installer::kLnkExt);
-    expected_path = expected_path.Append(shortcut_name);
+    return expected_path.Append(shortcut_name);
+  }
+
+  // Validates that the shortcut at |location| matches |properties| (and
+  // implicit default properties) for |dist|.
+  // Note: This method doesn't verify the |pin_to_taskbar| property as it
+  // implies real (non-mocked) state which is flaky to test.
+  void ValidateChromeShortcut(
+      ShellUtil::ShortcutLocation location,
+      BrowserDistribution* dist,
+      const ShellUtil::ShortcutProperties& properties) {
+    base::FilePath expected_path(
+        GetExpectedShortcutPath(location, dist, properties));
 
     base::win::ShortcutProperties expected_properties;
     if (properties.has_target()) {
@@ -143,7 +153,7 @@ class ShellUtilShortcutTest : public testing::Test {
       expected_properties.set_description(dist->GetAppDescription());
 
     if (properties.has_icon()) {
-      expected_properties.set_icon(properties.icon, 0);
+      expected_properties.set_icon(properties.icon, properties.icon_index);
     } else {
       int icon_index = dist->GetIconIndex(BrowserDistribution::SHORTCUT_CHROME);
       expected_properties.set_icon(chrome_exe_, icon_index);
@@ -186,6 +196,7 @@ class ShellUtilShortcutTest : public testing::Test {
 
   base::FilePath chrome_exe_;
   base::FilePath manganese_exe_;
+  base::FilePath other_ico_;
 };
 
 }  // namespace
@@ -368,11 +379,8 @@ TEST_F(ShellUtilShortcutTest, RemoveChromeShortcut) {
   ASSERT_TRUE(ShellUtil::CreateOrUpdateShortcut(
                   ShellUtil::SHORTCUT_LOCATION_DESKTOP, dist_, test_properties_,
                   ShellUtil::SHELL_SHORTCUT_CREATE_ALWAYS));
-
-  string16 shortcut_name(
-      dist_->GetShortcutName(BrowserDistribution::SHORTCUT_CHROME) +
-      installer::kLnkExt);
-  base::FilePath shortcut_path(fake_user_desktop_.path().Append(shortcut_name));
+  base::FilePath shortcut_path = GetExpectedShortcutPath(
+      ShellUtil::SHORTCUT_LOCATION_DESKTOP, dist_, test_properties_);
   ASSERT_TRUE(base::PathExists(shortcut_path));
 
   ASSERT_TRUE(ShellUtil::RemoveShortcuts(
@@ -387,12 +395,8 @@ TEST_F(ShellUtilShortcutTest, RemoveSystemLevelChromeShortcut) {
   ASSERT_TRUE(ShellUtil::CreateOrUpdateShortcut(
                   ShellUtil::SHORTCUT_LOCATION_DESKTOP, dist_, test_properties_,
                   ShellUtil::SHELL_SHORTCUT_CREATE_ALWAYS));
-
-  string16 shortcut_name(
-      dist_->GetShortcutName(BrowserDistribution::SHORTCUT_CHROME) +
-      installer::kLnkExt);
-  base::FilePath shortcut_path(
-      fake_common_desktop_.path().Append(shortcut_name));
+  base::FilePath shortcut_path = GetExpectedShortcutPath(
+      ShellUtil::SHORTCUT_LOCATION_DESKTOP, dist_, test_properties_);
   ASSERT_TRUE(base::PathExists(shortcut_path));
 
   ASSERT_TRUE(ShellUtil::RemoveShortcuts(
@@ -410,10 +414,8 @@ TEST_F(ShellUtilShortcutTest, RemoveMultipleChromeShortcuts) {
   ASSERT_TRUE(ShellUtil::CreateOrUpdateShortcut(
                   ShellUtil::SHORTCUT_LOCATION_DESKTOP, dist_, test_properties_,
                   ShellUtil::SHELL_SHORTCUT_CREATE_ALWAYS));
-  string16 shortcut1_name(
-      string16(kShortcutName1).append(installer::kLnkExt));
-  base::FilePath shortcut1_path(
-      fake_user_desktop_.path().Append(shortcut1_name));
+  base::FilePath shortcut1_path = GetExpectedShortcutPath(
+      ShellUtil::SHORTCUT_LOCATION_DESKTOP, dist_, test_properties_);
   ASSERT_TRUE(base::PathExists(shortcut1_path));
 
   test_properties_.set_shortcut_name(kShortcutName2);
@@ -421,9 +423,8 @@ TEST_F(ShellUtilShortcutTest, RemoveMultipleChromeShortcuts) {
   ASSERT_TRUE(ShellUtil::CreateOrUpdateShortcut(
                   ShellUtil::SHORTCUT_LOCATION_DESKTOP, dist_, test_properties_,
                   ShellUtil::SHELL_SHORTCUT_CREATE_ALWAYS));
-  string16 shortcut2_name(string16(kShortcutName2).append(installer::kLnkExt));
-  base::FilePath shortcut2_path(
-      fake_user_desktop_.path().Append(shortcut2_name));
+  base::FilePath shortcut2_path = GetExpectedShortcutPath(
+      ShellUtil::SHORTCUT_LOCATION_DESKTOP, dist_, test_properties_);
   ASSERT_TRUE(base::PathExists(shortcut2_path));
 
   ASSERT_TRUE(ShellUtil::RemoveShortcuts(
@@ -434,24 +435,18 @@ TEST_F(ShellUtilShortcutTest, RemoveMultipleChromeShortcuts) {
   ASSERT_TRUE(base::PathExists(shortcut1_path.DirName()));
 }
 
-TEST_F(ShellUtilShortcutTest, UpdateChromeShortcutsWithArgs) {
+TEST_F(ShellUtilShortcutTest, RetargetShortcutsWithArgs) {

[gab, 2014/01/03 18:25:52]

Seems a test is missing to make sure shortcuts that don't point to old_exe
aren't retargeted to new_exe; can you add this test as well (either to an
existing test or as a new one, as you think is best).

[huangs, 2014/01/03 20:19:35]

Added to the multi-case: {targets "iron.exe", icon "chrome.exe"}. Trying to
retarget chrome.exe" => "manganese.exe" has no effect.

Also added similar test case for shortcut removal (so a dumb algorithm that just
deletes everything will fail).

   ASSERT_TRUE(ShellUtil::CreateOrUpdateShortcut(
                   ShellUtil::SHORTCUT_LOCATION_DESKTOP, dist_, test_properties_,
                   ShellUtil::SHELL_SHORTCUT_CREATE_ALWAYS));
-
-  string16 shortcut_name(
-      dist_->GetShortcutName(BrowserDistribution::SHORTCUT_CHROME) +
-      installer::kLnkExt);
-  base::FilePath shortcut_path(fake_user_desktop_.path().Append(shortcut_name));
-  ASSERT_TRUE(base::PathExists(shortcut_path));
+  ASSERT_TRUE(base::PathExists(GetExpectedShortcutPath(
+      ShellUtil::SHORTCUT_LOCATION_DESKTOP, dist_, test_properties_)));
 
   base::FilePath new_exe = temp_dir_.path().Append(kManganeseExe);
-  ShellUtil::ShortcutProperties updated_properties(ShellUtil::CURRENT_USER);
-  updated_properties.set_target(new_exe);
-  // |updated_properties| has arguments.
-  ASSERT_TRUE(ShellUtil::UpdateShortcutsWithArgs(
+  // Relies on fact that |test_properties_| has non-empty arguments.
+  ASSERT_TRUE(ShellUtil::RetargetShortcutsWithArgs(
       ShellUtil::SHORTCUT_LOCATION_DESKTOP, dist_, ShellUtil::CURRENT_USER,
-      chrome_exe_, updated_properties));
+      chrome_exe_, new_exe));
 
   ShellUtil::ShortcutProperties expected_properties(test_properties_);
   expected_properties.set_target(new_exe);
@@ -459,26 +454,19 @@ TEST_F(ShellUtilShortcutTest, UpdateChromeShortcutsWithArgs) {
                          expected_properties);
 }
 
-TEST_F(ShellUtilShortcutTest, UpdateSystemLevelChromeShortcutsWithArgs) {
+TEST_F(ShellUtilShortcutTest, RetargetSystemLevelChromeShortcutsWithArgs) {
   test_properties_.level = ShellUtil::SYSTEM_LEVEL;
   ASSERT_TRUE(ShellUtil::CreateOrUpdateShortcut(
                   ShellUtil::SHORTCUT_LOCATION_DESKTOP, dist_, test_properties_,
                   ShellUtil::SHELL_SHORTCUT_CREATE_ALWAYS));
-
-  string16 shortcut_name(
-      dist_->GetShortcutName(BrowserDistribution::SHORTCUT_CHROME) +
-      installer::kLnkExt);
-  base::FilePath shortcut_path(
-      fake_common_desktop_.path().Append(shortcut_name));
-  ASSERT_TRUE(base::PathExists(shortcut_path));
+  ASSERT_TRUE(base::PathExists(GetExpectedShortcutPath(
+      ShellUtil::SHORTCUT_LOCATION_DESKTOP, dist_, test_properties_)));
 
   base::FilePath new_exe = temp_dir_.path().Append(kManganeseExe);
-  ShellUtil::ShortcutProperties updated_properties(ShellUtil::CURRENT_USER);
-  updated_properties.set_target(new_exe);
-  // |updated_properties| has arguments.
-  ASSERT_TRUE(ShellUtil::UpdateShortcutsWithArgs(
+  // Relies on fact that |test_properties_| has non-empty arguments.
+  ASSERT_TRUE(ShellUtil::RetargetShortcutsWithArgs(
       ShellUtil::SHORTCUT_LOCATION_DESKTOP, dist_, ShellUtil::SYSTEM_LEVEL,
-      chrome_exe_, updated_properties));
+      chrome_exe_, new_exe));
 
   ShellUtil::ShortcutProperties expected_properties(test_properties_);
   expected_properties.set_target(new_exe);
@@ -486,7 +474,7 @@ TEST_F(ShellUtilShortcutTest, UpdateSystemLevelChromeShortcutsWithArgs) {
                          expected_properties);
 }
 
-TEST_F(ShellUtilShortcutTest, UpdateMultipleChromeShortcutsWithArgs) {
+TEST_F(ShellUtilShortcutTest, RetargetChromeShortcutsWithArgsEmpty) {
   const wchar_t kShortcutName1[] = L"Chrome 1";
   const wchar_t kShortcutName2[] = L"Chrome 2";
 
@@ -496,40 +484,75 @@ TEST_F(ShellUtilShortcutTest, UpdateMultipleChromeShortcutsWithArgs) {
   ASSERT_TRUE(ShellUtil::CreateOrUpdateShortcut(
                   ShellUtil::SHORTCUT_LOCATION_DESKTOP, dist_, test_properties_,
                   ShellUtil::SHELL_SHORTCUT_CREATE_ALWAYS));
-  string16 shortcut1_name(string16(kShortcutName1).append(installer::kLnkExt));
-  base::FilePath shortcut1_path(
-      fake_user_desktop_.path().Append(shortcut1_name));
+  ASSERT_TRUE(base::PathExists(GetExpectedShortcutPath(
+      ShellUtil::SHORTCUT_LOCATION_DESKTOP, dist_, test_properties_)));
   ShellUtil::ShortcutProperties expected_properties1(test_properties_);
 
   // Setup shortcut 2, which has non-empty arguments.
-  string16 shortcut2_args = L"--profile-directory=\"Profile 2\"";
   test_properties_.set_shortcut_name(kShortcutName2);
-  test_properties_.set_arguments(shortcut2_args);
+  test_properties_.set_arguments(L"--profile-directory=\"Profile 2\"");
   ASSERT_TRUE(ShellUtil::CreateOrUpdateShortcut(
                   ShellUtil::SHORTCUT_LOCATION_DESKTOP, dist_, test_properties_,
                   ShellUtil::SHELL_SHORTCUT_CREATE_ALWAYS));
-  string16 shortcut2_name(string16(kShortcutName2).append(installer::kLnkExt));
-  base::FilePath shortcut2_path(
-      fake_user_desktop_.path().Append(shortcut2_name));
-  ASSERT_TRUE(base::PathExists(shortcut2_path));
+  ASSERT_TRUE(base::PathExists(GetExpectedShortcutPath(
+      ShellUtil::SHORTCUT_LOCATION_DESKTOP, dist_, test_properties_)));
   ShellUtil::ShortcutProperties expected_properties2(test_properties_);
 
-  // Update shortcuts: target "manganese.exe" instead of "chrome.exe".
+  // Retarget shortcuts: replace "chrome.exe" with "manganese.exe". Only
+  // shortcuts with non-empty arguments (i.e., shortcut 2) gets updated.
   base::FilePath new_exe = temp_dir_.path().Append(kManganeseExe);
-  ShellUtil::ShortcutProperties updated_properties(ShellUtil::CURRENT_USER);
-  updated_properties.set_target(new_exe);
-
-  // Only changing shrotcuts that have non-empty arguments, i.e., shortcut 2.
-  ASSERT_TRUE(ShellUtil::UpdateShortcutsWithArgs(
+  ASSERT_TRUE(ShellUtil::RetargetShortcutsWithArgs(
       ShellUtil::SHORTCUT_LOCATION_DESKTOP, dist_, ShellUtil::CURRENT_USER,
-      chrome_exe_, updated_properties));
+      chrome_exe_, new_exe));
   // Verify shortcut 1.
-  // |expected_properties1| was unchanged and still targets "chrome.exe", since
+  // |expected_properties1| is unchanged and still targets "chrome.exe", since
   // it has empty target, yet we passed |require_args| = true.

[gab, 2014/01/03 18:25:52]

|require_args| no longer exists right? please update this comment

[huangs, 2014/01/03 20:19:35]

Done.

   ValidateChromeShortcut(ShellUtil::SHORTCUT_LOCATION_DESKTOP, dist_,
                          expected_properties1);
-  // Verify shortcut 2.
+  // Verify shortcut 2, which now targets "manganese.exe".
+  expected_properties2.set_target(new_exe);
+  ValidateChromeShortcut(ShellUtil::SHORTCUT_LOCATION_DESKTOP, dist_,
+                         expected_properties2);
+}
+
+TEST_F(ShellUtilShortcutTest, RetargetChromeShortcutsWithArgsIcon) {
+  const wchar_t kShortcutName1[] = L"Chrome 1";
+  const wchar_t kShortcutName2[] = L"Chrome 2";
+
+  // Setup shortcut 1, which has icon set to "chrome.exe".
+  test_properties_.set_shortcut_name(kShortcutName1);
+  test_properties_.set_icon(chrome_exe_, 3);

[gab, 2014/01/03 18:25:52]

Use test_properties_.target instead of chrome_exe_ to be explicit about the fact
that it's important this be the same exe.

[huangs, 2014/01/03 20:19:35]

Done.

+  ASSERT_TRUE(ShellUtil::CreateOrUpdateShortcut(
+                  ShellUtil::SHORTCUT_LOCATION_DESKTOP, dist_, test_properties_,
+                  ShellUtil::SHELL_SHORTCUT_CREATE_ALWAYS));
+  ASSERT_TRUE(base::PathExists(GetExpectedShortcutPath(
+      ShellUtil::SHORTCUT_LOCATION_DESKTOP, dist_, test_properties_)));
+  ShellUtil::ShortcutProperties expected_properties1(test_properties_);
+
+  // Setup shortcut 2, which has icon set to "other.ico".
+  test_properties_.set_shortcut_name(kShortcutName2);
+  test_properties_.set_icon(other_ico_, 0);
+  ASSERT_TRUE(ShellUtil::CreateOrUpdateShortcut(
+                  ShellUtil::SHORTCUT_LOCATION_DESKTOP, dist_, test_properties_,
+                  ShellUtil::SHELL_SHORTCUT_CREATE_ALWAYS));
+  ASSERT_TRUE(base::PathExists(GetExpectedShortcutPath(
+      ShellUtil::SHORTCUT_LOCATION_DESKTOP, dist_, test_properties_)));
+  ShellUtil::ShortcutProperties expected_properties2(test_properties_);
+
+  // Retarget shortcuts: replace "chrome.exe" with "manganese.exe".
+  // Relies on fact that |test_properties_| has non-empty arguments.

[gab, 2014/01/03 18:25:52]

s/Relies on fact/Relies on the fact

Here and elsewhere in this file

[huangs, 2014/01/03 20:19:35]

Done.

+  base::FilePath new_exe = temp_dir_.path().Append(kManganeseExe);
+  ASSERT_TRUE(ShellUtil::RetargetShortcutsWithArgs(
+      ShellUtil::SHORTCUT_LOCATION_DESKTOP, dist_, ShellUtil::CURRENT_USER,
+      chrome_exe_, new_exe));
+  // Verify shortcut 1: icon now targets "manganese.exe", with same icon index.
+  expected_properties1.set_target(new_exe);
+  expected_properties1.set_icon(new_exe, 3);

[gab, 2014/01/03 18:25:52]

Move '3' into a local scope constant to be used here and on line 524.

Same for '0' for shortcut 2 (for which I suggest using some index other than
'0').

[huangs, 2014/01/03 20:19:35]

Done. The consts for the other 2 cases are only used once, but they deserve
const since they are the main thing to test.

By contrast, distinct shortcut names "Chrome 1", "Chrome 2", "Iron 3", are just
used only once, so no longer stored in variable.

+  ValidateChromeShortcut(ShellUtil::SHORTCUT_LOCATION_DESKTOP, dist_,
+                         expected_properties1);
+  // Verify shortcut 2: icon remains unchanged.
   expected_properties2.set_target(new_exe);
+  expected_properties2.set_icon(other_ico_, 0);
   ValidateChromeShortcut(ShellUtil::SHORTCUT_LOCATION_DESKTOP, dist_,
                          expected_properties2);
 }