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..7f4912c2e5e3334f38a1bf7ed2b87385b994b9b8 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,6 +86,29 @@ class ShellUtilShortcutTest : public testing::Test {
     test_properties_.set_dual_mode(true);
   }
 
+  // Helper to create a shortcut on the desktop, using |test_properties_|. Also
+  // asserts that creation was successful. |shortcut_path_out| is an optional
+  // output parameter, for the path to the shortcut created.
+  void CreateTestDesktopShortcut(base::FilePath* shortcut_path_out) {

[huangs, 2013/12/30 20:19:32]

Cannot simply return base::FilePath, because ASSERT_TRUE() triggers the error
message

error C2664: 'base::FilePath::FilePath(const base::FilePath &)' : cannot convert
parameter 1 from 'void' to 'const base::FilePath &'

Expressions of type void cannot be converted to other types

[gab, 2014/01/02 15:51:46]

Right, that's because ASSERTs force a "return;", always use EXPECTs in helper
test methods for that reason (ASSERTs in helper methods makes it look like the
test will end on failure which is not true... it'll log an error and return from
the helper method only, not exit the test).

If you want to react differently on the result, then store the result and do 

EXPECT_TRUE(result);
if (!result)
  return base::FilePath();


Either way... reading this some more I find the previous code easier to read
than with this helper method.

I find the helper adds complexity as it acts based on test_properties_ (which is
a member not a method param so it's not obvious) to support various independent
use cases.

I understand why you want to do this, but I think the minimal code duplication
in each test was better than this, please change it back.

Feel free to create a "default_shortcut_name" string variable if you want to
turn blocks like

   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));

into one-liner

ASSERT_TRUE(base::PathExists(fake_user_desktop.path().Append(default_shortcut_name_)));


Than each test has creation calls + matching verification calls; this doesn't
feel like it needs a helper method to me.

[huangs, 2014/01/02 19:58:22]

Key changes:
- Returning base::FilePath now.
- Checking for existence outside this routine.
- The shortcut path code is extracted from ValidateChromeShortcut() into 
GetExpectedShortcutPath(), and is called in CreateTestDesktopShortcut().

+    ASSERT_TRUE(ShellUtil::CreateOrUpdateShortcut(
+                    ShellUtil::SHORTCUT_LOCATION_DESKTOP,
+                    dist_,
+                    test_properties_,
+                    ShellUtil::SHELL_SHORTCUT_CREATE_ALWAYS));
+    // Compute |shortcut_path|, deducing specified or default values.
+    const base::FilePath& base_path(
+        (test_properties_.level == ShellUtil::CURRENT_USER) ?
+            fake_user_desktop_.path() : fake_common_desktop_.path());
+    string16 shortcut_name = test_properties_.has_shortcut_name() ?
+        test_properties_.shortcut_name :
+            dist_->GetShortcutName(BrowserDistribution::SHORTCUT_CHROME);
+    base::FilePath shortcut_path(
+        base_path.Append(shortcut_name + installer::kLnkExt));
+    ASSERT_TRUE(base::PathExists(shortcut_path));
+    if (shortcut_path_out)
+      *shortcut_path_out = shortcut_path;
+  }
+
   // 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
@@ -143,7 +170,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 +213,7 @@ class ShellUtilShortcutTest : public testing::Test {
 
   base::FilePath chrome_exe_;
   base::FilePath manganese_exe_;
+  base::FilePath other_ico_;
 };
 
 }  // namespace
@@ -365,15 +393,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(&shortcut_path);
 
   ASSERT_TRUE(ShellUtil::RemoveShortcuts(
       ShellUtil::SHORTCUT_LOCATION_DESKTOP, dist_, ShellUtil::CURRENT_USER,
@@ -384,16 +405,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(&shortcut_path);
 
   ASSERT_TRUE(ShellUtil::RemoveShortcuts(
       ShellUtil::SHORTCUT_LOCATION_DESKTOP, dist_, ShellUtil::SYSTEM_LEVEL,
@@ -407,24 +420,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(&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(&shortcut2_path);
 
   ASSERT_TRUE(ShellUtil::RemoveShortcuts(
       ShellUtil::SHORTCUT_LOCATION_DESKTOP, dist_, ShellUtil::CURRENT_USER,
@@ -434,24 +436,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) {
+  CreateTestDesktopShortcut(NULL);
 
   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 +451,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));
+  CreateTestDesktopShortcut(NULL);
 
   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 +467,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));
+  CreateTestDesktopShortcut(NULL);
   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\"");
+  CreateTestDesktopShortcut(NULL);
   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);
+  CreateTestDesktopShortcut(NULL);
+  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);
+  CreateTestDesktopShortcut(NULL);
+  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/02 15:51:46]

To strengthen my point about not needing the helper method; I find this more
explicit when the call to create the shortcut via test_properties_ is explicitly
in this test.

[huangs, 2014/01/02 19:58:22]

Passing test_properties_ as parameter to the routine.

+  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);
 }