Find shortcuts that don't have a profile command line set when creating 2nd profile.

chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc

Index: chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc
===================================================================
--- chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc	(revision 169477)
+++ chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc	(working copy)
@@ -16,6 +16,7 @@
 #include "chrome/browser/profiles/profile_shortcut_manager.h"
 #include "chrome/browser/profiles/profile_shortcut_manager_win.h"
 #include "chrome/installer/util/browser_distribution.h"
+#include "chrome/installer/util/product.h"
 #include "chrome/installer/util/shell_util.h"
 #include "chrome/test/base/testing_browser_process.h"
 #include "chrome/test/base/testing_profile.h"
@@ -104,19 +105,23 @@
     ValidateProfileShortcut(string16());
   }
 
+  void CreateSecondProfileWithShortcut() {
+    ASSERT_FALSE(ProfileShortcutExistsAtDefaultPath(second_profile_name_));
+    profile_info_cache_->AddProfileToCache(second_dest_path_,
+                                           second_profile_name_, string16(), 0);
+    profile_shortcut_manager_->CreateProfileShortcut(second_dest_path_);

[sail, 2012/11/26 21:27:16]

I know this was there previously but is it really necessary? Shouldn't the
shortcut manager get a notification that a profile was added and create a
shortcut automatically?

[Alexei Svitkine (slow), 2012/11/27 14:49:26]

No. Whether the shortcut is created is based on the value of the checkbox in the
UI normally - so the notification doesn't carry enough information on whether
the shortcut should be created.

It's the web UI handler code that calls into this to create the shortcut after
creating the profile. That's equivalent to what's being done here.

+    RunPendingTasks();
+    ValidateProfileShortcut(second_profile_name_);
+  }
+
   void SetupAndCreateTwoShortcuts() {
     ASSERT_EQ(0, profile_info_cache_->GetNumberOfProfiles());
     ASSERT_FALSE(ProfileShortcutExistsAtDefaultPath(profile_name_));
-    ASSERT_FALSE(ProfileShortcutExistsAtDefaultPath(second_profile_name_));
 
     profile_info_cache_->AddProfileToCache(dest_path_, profile_name_,
                                            string16(), 0);
-    profile_info_cache_->AddProfileToCache(second_dest_path_,
-                                           second_profile_name_, string16(), 0);
-    profile_shortcut_manager_->CreateProfileShortcut(second_dest_path_);
-    RunPendingTasks();
+    CreateSecondProfileWithShortcut();
     ValidateProfileShortcut(profile_name_);
-    ValidateProfileShortcut(second_profile_name_);
   }
 
   // Returns the default shortcut path for this profile.
@@ -148,6 +153,24 @@
     base::win::ValidateShortcut(shortcut_path, expected_properties);
   }
 
+  // Creates a regular (non-profile) desktop shortcut with the given name and
+  // returns its path. Fails the test if an error occurs.
+  FilePath CreateRegularShortcutWithName(const string16& shortcut_name) {
+    const FilePath shortcut_path =
+        shortcuts_directory_.Append(shortcut_name + installer::kLnkExt);
+    EXPECT_FALSE(file_util::PathExists(shortcut_path));
+
+    installer::Product product(distribution_);
+    ShellUtil::ShortcutProperties properties(ShellUtil::CURRENT_USER);
+    product.AddDefaultShortcutProperties(exe_path_, &properties);
+    properties.set_shortcut_name(shortcut_name);
+    EXPECT_TRUE(ShellUtil::CreateOrUpdateShortcut(
+        ShellUtil::SHORTCUT_LOCATION_DESKTOP, distribution_, properties,
+        ShellUtil::SHELL_SHORTCUT_CREATE_ALWAYS));
+
+    return shortcut_path;
+  }
+
   BrowserDistribution* distribution_;
   MessageLoopForUI message_loop_;
   content::TestBrowserThread ui_thread_;
@@ -180,7 +203,7 @@
 }
 
 TEST_F(ProfileShortcutManagerTest, DesktopShortcutsCreate) {
-  ProfileShortcutManagerTest::SetupDefaultProfileShortcut();
+  SetupDefaultProfileShortcut();
 
   profile_info_cache_->AddProfileToCache(second_dest_path_,
                                          second_profile_name_, string16(), 0);
@@ -195,7 +218,7 @@
 }
 
 TEST_F(ProfileShortcutManagerTest, DesktopShortcutsUpdate) {
-  ProfileShortcutManagerTest::SetupDefaultProfileShortcut();
+  SetupDefaultProfileShortcut();
 
   EXPECT_FALSE(ProfileShortcutExistsAtDefaultPath(second_profile_name_));
 
@@ -217,7 +240,7 @@
 }
 
 TEST_F(ProfileShortcutManagerTest, DesktopShortcutsDeleteSecondToLast) {
-  ProfileShortcutManagerTest::SetupAndCreateTwoShortcuts();
+  SetupAndCreateTwoShortcuts();
 
   // Delete one shortcut.
   profile_info_cache_->DeleteProfileFromCache(second_dest_path_);
@@ -232,7 +255,7 @@
 }
 
 TEST_F(ProfileShortcutManagerTest, DesktopShortcutsCreateSecond) {
-  ProfileShortcutManagerTest::SetupAndCreateTwoShortcuts();
+  SetupAndCreateTwoShortcuts();
 
   // Delete one shortcut.
   profile_info_cache_->DeleteProfileFromCache(second_dest_path_);
@@ -258,7 +281,7 @@
 }
 
 TEST_F(ProfileShortcutManagerTest, RenamedDesktopShortcuts) {
-  ProfileShortcutManagerTest::SetupAndCreateTwoShortcuts();
+  SetupAndCreateTwoShortcuts();
 
   const FilePath old_shortcut_path =
       GetDefaultShortcutPathForProfile(second_profile_name_);
@@ -281,7 +304,7 @@
 }
 
 TEST_F(ProfileShortcutManagerTest, RenamedDesktopShortcutsGetDeleted) {
-  ProfileShortcutManagerTest::SetupAndCreateTwoShortcuts();
+  SetupAndCreateTwoShortcuts();
 
   const FilePath old_shortcut_path =
       GetDefaultShortcutPathForProfile(second_profile_name_);
@@ -309,7 +332,7 @@
 }
 
 TEST_F(ProfileShortcutManagerTest, RenamedDesktopShortcutsAfterProfileRename) {
-  ProfileShortcutManagerTest::SetupAndCreateTwoShortcuts();
+  SetupAndCreateTwoShortcuts();
 
   const FilePath old_shortcut_path =
       GetDefaultShortcutPathForProfile(second_profile_name_);
@@ -338,3 +361,68 @@
   EXPECT_TRUE(file_util::PathExists(updated_shortcut_path));
 }
 
+TEST_F(ProfileShortcutManagerTest, UpdateShortcutWithNoFlags) {
+  SetupDefaultProfileShortcut();
+
+  // Delete the shortcut that got created for this profile and instead make
+  // a new one without any command-line flags.
+  ASSERT_TRUE(file_util::Delete(GetDefaultShortcutPathForProfile(string16()),
+                                false));
+  const FilePath default_shortcut_path =

[sail, 2012/11/26 21:27:16]

naming here is inconsistent.
above you refer to the shortcut for the first profile as the default shortcut
here you're referring to the shortcut that points to Chrome and not any profile.

maybe rename this to regular_shortcut_path (same below)

[Alexei Svitkine (slow), 2012/11/27 22:31:26]

Done.

+      CreateRegularShortcutWithName(distribution_->GetAppShortCutName());
+  EXPECT_TRUE(file_util::PathExists(default_shortcut_path));

[sail, 2012/11/26 21:27:16]

move this into CreateRegularShortcutWithName? (same below)

[Alexei Svitkine (slow), 2012/11/27 14:49:26]

The reason I didn't is because when an EXPECT fails in a helper function, it
doesn't give the stack trace of where it was called. (Thus, if there was a
failure, I wouldn't know if it was the first CreateRegularShortcutWithName() or
the second one that failed in this test case.)

This problem already plagues these tests, since there's a lot of checking done
in helper functions.

Unless there's a good way to preserve the callsite, I'd rather not tuck things
away even more. (If there is a good way to preserve the callsite, then by all
means I agree.)

I guess one possibility would be to pass __FILE__ and __LINE__ to all the
helpers, which would log them on failure. Seems messy, though. What do you
think?

[Alexei Svitkine (slow), 2012/11/27 14:53:58]

Replying to myself:

Looks like we have base/location.h which could work. We can use the FROM_HERE
macro from that file to get a tracked_objects::Location object that could be
passed to all the helper functions as a parameter. Then, their EXPECT lines
could do: "EXPEXT_TRUE(foo) << location.ToString();".

It sounds like that may do the trick. WDYT?

[Alexei Svitkine (slow), 2012/11/27 22:31:26]

Done.

+
+  // Add another profile and check that the shortcut was renamed.
+  CreateSecondProfileWithShortcut();
+
+  EXPECT_FALSE(file_util::PathExists(default_shortcut_path));
+  const FilePath new_shortcut_path =
+      GetDefaultShortcutPathForProfile(profile_name_);

[sail, 2012/11/26 21:27:16]

This test is really confusing.

Here's what I think you're doing:
  #1 Call SetupDefaultProfileShortcut which
     #1a. creates a first profile (named profile_name_ and located at
dest_path_)
     #1b. creates a shortcut that points to the first profile
  #2. Delete the shortcut that points to the first profile
  #3. Create a shortcut that just launches Chrome and doesn't point to any
profile
  #4 Call CreateSecondProfileWithShortcut which
     #4a. creates a second profile (named second_profile_name_ and located at
second_dest_path_)
     #4b. creates a shortcut that points to the second profile
  #5 Check that the shortcut for the first profile exists (it was previously
deleted in step #2)
  #6 Check that the shortcut for profile 1 has the correct flags

I think there are several things you could do to make this easier to follow:
  - rename variables like dest_path_ and second_dest_path_ to something like
profile_1_path_ and profile_2_path_. Same with profile_name_, maybe rename to
profile_1_name_.
  - come up with a better way to distinguish between shortcuts that point to a
profile and shortcuts that don't point to a profile. Currently you use "default"
and "regular" which is confusing.
  - Move the shortcut verification into a separate helper function. Maybe
VerifyProfile1Shortcut() and VerifyShortcutForProfile2(). This can both check
that the shortcut exists and that the command line arguments are correct.

With that done you could write this test like this:
  ASSERT(profile count == 0)
  profile_1_shortcut_path = CreateProfile1AndShortcut(); // (also verifies the
shortcut)
  Delete(profile_1_shortcut_path)
  chrome_only_shortcut_path = CreateChromeOnlyShortcut()
  CreateProfile2AndShortcut(); // (also verifies the shortcut)
  EXPECT_FALSE(PathExists(chrome_only_shortcut_path))
  VerifyProfile2Shortcut()

[Alexei Svitkine (slow), 2012/11/27 22:31:26]

That's correct.
Done. This was bothering me too.
I'm changed the test to be consistent with the use of the name "regular",
instead of default, which was an overloaded term in the test.
I still need to do this. Right now, we have ValidateProfileShortcut(), which
does a bunch of validation already, but I've not yet incorporated the command
line checking into it. I'll do this tomorrow.

+  EXPECT_TRUE(file_util::PathExists(new_shortcut_path));
+
+  // Check that the renamed shortcut has the correct command line flags set.
+  string16 shortcut_args;
+  ASSERT_TRUE(base::win::ResolveShortcut(new_shortcut_path, NULL,
+                                         &shortcut_args));
+  const string16 expected_args =
+      profiles::internal::CreateProfileShortcutFlags(dest_path_);
+  EXPECT_NE(string16::npos, shortcut_args.find(expected_args));
+}
+
+TEST_F(ProfileShortcutManagerTest, UpdateTwoShortcutsWithNoFlags) {
+  SetupDefaultProfileShortcut();
+
+  // Delete the shortcut that got created for this profile and instead make
+  // two new ones without any command-line flags.
+  ASSERT_TRUE(file_util::Delete(GetDefaultShortcutPathForProfile(string16()),
+                                false));
+  const FilePath default_shortcut_path =
+      CreateRegularShortcutWithName(distribution_->GetAppShortCutName());
+  EXPECT_TRUE(file_util::PathExists(default_shortcut_path));
+  const FilePath second_shortcut_path =
+      CreateRegularShortcutWithName(L"MyChrome");
+  EXPECT_TRUE(file_util::PathExists(second_shortcut_path));
+
+  // Add another profile and check that one shortcut was renamed and that the
+  // other shortcut was updated but kept the same name.
+  CreateSecondProfileWithShortcut();
+  EXPECT_FALSE(file_util::PathExists(default_shortcut_path));
+  EXPECT_TRUE(file_util::PathExists(second_shortcut_path));
+
+  const FilePath new_shortcut_path =
+      GetDefaultShortcutPathForProfile(profile_name_);
+  EXPECT_TRUE(file_util::PathExists(new_shortcut_path));
+
+  // Check that both shortcuts have the correct command line flags set.
+  const string16 expected_args =
+      profiles::internal::CreateProfileShortcutFlags(dest_path_);
+  string16 shortcut_args;
+  ASSERT_TRUE(base::win::ResolveShortcut(new_shortcut_path, NULL,
+                                         &shortcut_args));
+  EXPECT_NE(string16::npos, shortcut_args.find(expected_args));
+
+  shortcut_args.clear();
+  ASSERT_TRUE(base::win::ResolveShortcut(second_shortcut_path, NULL,
+                                         &shortcut_args));
+  EXPECT_NE(string16::npos, shortcut_args.find(expected_args));
+}