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..0aae6ce609089c876588b771fec18c1b836afe70 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,36 @@ 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);

[gab, 2014/01/02 21:01:46]

I typically indent the two items of a ternary operator at the same level (just
like with &&/||).

i.e., unindent 4 spaces.

[huangs, 2014/01/02 23:22:37]

Done.

     shortcut_name.append(installer::kLnkExt);
-    expected_path = expected_path.Append(shortcut_name);
+    return expected_path.Append(shortcut_name);
+  }
+
+  // Helper to create a test shortcut on the (fake) desktop, and returns the
+  // path to the new shortcut.
+  base::FilePath CreateTestDesktopShortcut(
+      const ShellUtil::ShortcutProperties& properties) {
+    ShellUtil::ShortcutLocation location = ShellUtil::SHORTCUT_LOCATION_DESKTOP;
+    EXPECT_TRUE(ShellUtil::CreateOrUpdateShortcut(
+        location, dist_, properties, ShellUtil::SHELL_SHORTCUT_CREATE_ALWAYS));
+    return GetExpectedShortcutPath(location, dist_, properties);
+  }
+
+  // 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 +163,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 +206,7 @@ class ShellUtilShortcutTest : public testing::Test {
 
   base::FilePath chrome_exe_;
   base::FilePath manganese_exe_;
+  base::FilePath other_ico_;
 };
 
 }  // namespace
@@ -365,15 +386,8 @@ TEST_F(ShellUtilShortcutTest, CreateAlwaysUserWithSystemLevelPresent) {
 }
 
 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));
-  ASSERT_TRUE(base::PathExists(shortcut_path));
+  base::FilePath shortcut_path = CreateTestDesktopShortcut(test_properties_);

[gab, 2014/01/02 21:01:46]

So I really like that the refactoring is getting rid of building the
shortcut_name everywhere by extracting GetExpectedShortcutPath() out of
ValidateChromeShortcut(), but I still don't like the helper method
(CreateTestDesktopShortcut()) itself because:

1) It hides the calling convention to ShellUtil::CreateOrUpdateShortcut; I like
having it here as it is closely related to, e.g., the call to
ShellUtil::RemoveShortcuts below; and it makes the test code easy to read.

2) The helper method really just bundles two independent things: 
 - Creating the shortcut
 - Getting the expected path
I don't see why a common method is required for both.

I think we get rid of most of the boiler plate with GetExpectedShortcutPath()
and don't think a special helper to create a specific type of shortcut
(user-level desktop shortcut) is necessary.

[huangs, 2014/01/02 23:22:37]

If we get rid of the routine, we would replace each

  EXPECT_TRUE(base::PathExists(CreateTestDesktopShortcut(test_properties_)));

with (some spacing fix may be needed):

  EXPECT_TRUE(ShellUtil::CreateOrUpdateShortcut(
                  ShellUtil::SHORTCUT_LOCATION_DESKTOP, dist_,
                  test_properties_, ShellUtil::SHELL_SHORTCUT_CREATE_ALWAYS));
  EXPECT_TRUE(base::PathExists(GetExpectedShortcutPath(
                  ShellUtil::SHORTCUT_LOCATION_DESKTOP, dist_,
                  test_properties_)));

The case where we save the path is similar. I like the helper method because:

(1) It shortens code and increases uniformity.
(2) It removes distraction from the "main point" of tests. Note that the
shortcut creation tests were left alone; anyone who wants to understand the
nuances of shortcut creation should look at those.
(3) It's easier to modify en mass, e.g., if we change param to
CreateOrUpdateShortcut(), we have fewer places to change.

As we both feel strongly about this, let's defer the decision to grt@.  :)

+  EXPECT_TRUE(base::PathExists(shortcut_path));
 
   ASSERT_TRUE(ShellUtil::RemoveShortcuts(
       ShellUtil::SHORTCUT_LOCATION_DESKTOP, dist_, ShellUtil::CURRENT_USER,
@@ -384,16 +398,8 @@ TEST_F(ShellUtilShortcutTest, RemoveChromeShortcut) {
 
 TEST_F(ShellUtilShortcutTest, RemoveSystemLevelChromeShortcut) {
   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));
+  base::FilePath shortcut_path = CreateTestDesktopShortcut(test_properties_);
+  EXPECT_TRUE(base::PathExists(shortcut_path));
 
   ASSERT_TRUE(ShellUtil::RemoveShortcuts(
       ShellUtil::SHORTCUT_LOCATION_DESKTOP, dist_, ShellUtil::SYSTEM_LEVEL,
@@ -407,24 +413,13 @@ TEST_F(ShellUtilShortcutTest, RemoveMultipleChromeShortcuts) {
   const wchar_t kShortcutName2[] = L"Chrome 2";
 
   test_properties_.set_shortcut_name(kShortcutName1);
-  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(shortcut1_path));
+  base::FilePath shortcut1_path = CreateTestDesktopShortcut(test_properties_);
+  EXPECT_TRUE(base::PathExists(shortcut1_path));
 
   test_properties_.set_shortcut_name(kShortcutName2);
   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));
+  base::FilePath shortcut2_path = CreateTestDesktopShortcut(test_properties_);
+  EXPECT_TRUE(base::PathExists(shortcut2_path));
 
   ASSERT_TRUE(ShellUtil::RemoveShortcuts(
       ShellUtil::SHORTCUT_LOCATION_DESKTOP, dist_, ShellUtil::CURRENT_USER,
@@ -434,24 +429,14 @@ TEST_F(ShellUtilShortcutTest, RemoveMultipleChromeShortcuts) {
   ASSERT_TRUE(base::PathExists(shortcut1_path.DirName()));
 }
 
-TEST_F(ShellUtilShortcutTest, UpdateChromeShortcutsWithArgs) {
-  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));
+TEST_F(ShellUtilShortcutTest, RetargetShortcutsWithArgs) {
+  EXPECT_TRUE(base::PathExists(CreateTestDesktopShortcut(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 +444,15 @@ 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));
+  EXPECT_TRUE(base::PathExists(CreateTestDesktopShortcut(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,50 +460,69 @@ 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";
 
   // Setup shortcut 1, which has empty arguments.
   test_properties_.set_shortcut_name(kShortcutName1);
   test_properties_.set_arguments(L"");
-  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));
+  EXPECT_TRUE(base::PathExists(CreateTestDesktopShortcut(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);
-  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));
+  test_properties_.set_arguments(L"--profile-directory=\"Profile 2\"");
+  EXPECT_TRUE(base::PathExists(CreateTestDesktopShortcut(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.
   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);
+  EXPECT_TRUE(base::PathExists(CreateTestDesktopShortcut(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);
+  EXPECT_TRUE(base::PathExists(CreateTestDesktopShortcut(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.
+  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);
+  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);
 }