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

chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include <objbase.h>  // For CoInitialize().
 
 #include "base/file_util.h"
 #include "base/message_loop.h"
 #include "base/path_service.h"
 #include "base/test/scoped_path_override.h"
 #include "base/string16.h"
 #include "base/test/test_shortcut_win.h"
 #include "base/win/shortcut.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #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"
 #include "chrome/test/base/testing_profile_manager.h"
 #include "content/public/test/test_browser_thread.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using content::BrowserThread;
 
 class ProfileShortcutManagerTest : public testing::Test {
@@ skipping 68 matching lines @@
     // A non-badged shortcut for chrome is automatically created with the
     // first profile (for the case when the user deletes their only profile).
     profile_info_cache_->AddProfileToCache(dest_path_, profile_name_,
                                            string16(), 0);
     RunPendingTasks();
     // We now have 1 profile, so we expect a new shortcut with no profile
     // information.
     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.
   FilePath GetDefaultShortcutPathForProfile(const string16& profile_name) {
     return shortcuts_directory_.Append(
         profiles::internal::GetShortcutFilenameForProfile(profile_name,
                                                           distribution_));
   }
 
   // Returns true if the shortcut for this profile exists.
@@ skipping 11 matching lines @@
 
     // TODO(asvitkine): With this new struct method for VerifyShortcut you can
     // now test more properties like: arguments, icon, icon_index, and app_id.
     base::win::ShortcutProperties expected_properties;
     expected_properties.set_target(exe_path_);
     expected_properties.set_description(distribution_->GetAppDescription());
     expected_properties.set_dual_mode(false);
     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_;
   content::TestBrowserThread file_thread_;
   scoped_ptr<TestingProfileManager> profile_manager_;
   scoped_ptr<ProfileShortcutManager> profile_shortcut_manager_;
   ProfileInfoCache* profile_info_cache_;
   base::ScopedPathOverride fake_user_desktop_;
   FilePath exe_path_;
   FilePath dest_path_;
@@ skipping 12 matching lines @@
                                                               distribution_));
 }
 
 TEST_F(ProfileShortcutManagerTest, UnbadgedShortcutFilename) {
   EXPECT_EQ(distribution_->GetAppShortCutName() + installer::kLnkExt,
             profiles::internal::GetShortcutFilenameForProfile(string16(),
                                                               distribution_));
 }
 
 TEST_F(ProfileShortcutManagerTest, DesktopShortcutsCreate) {
-  ProfileShortcutManagerTest::SetupDefaultProfileShortcut();
+  SetupDefaultProfileShortcut();
 
   profile_info_cache_->AddProfileToCache(second_dest_path_,
                                          second_profile_name_, string16(), 0);
   profile_shortcut_manager_->CreateProfileShortcut(second_dest_path_);
   RunPendingTasks();
 
   // We now have 2 profiles, so we expect a new shortcut with profile
   // information for this 2nd profile.
   ValidateProfileShortcut(second_profile_name_);
   ASSERT_TRUE(file_util::PathExists(second_dest_path_.Append(
       FILE_PATH_LITERAL("Google Profile.ico"))));
 }
 
 TEST_F(ProfileShortcutManagerTest, DesktopShortcutsUpdate) {
-  ProfileShortcutManagerTest::SetupDefaultProfileShortcut();
+  SetupDefaultProfileShortcut();
 
   EXPECT_FALSE(ProfileShortcutExistsAtDefaultPath(second_profile_name_));
 
   profile_info_cache_->AddProfileToCache(second_dest_path_,
                                          second_profile_name_, string16(), 0);
   profile_shortcut_manager_->CreateProfileShortcut(second_dest_path_);
   RunPendingTasks();
   ValidateProfileShortcut(second_profile_name_);
 
   // Cause an update in ProfileShortcutManager by modifying the profile info
   // cache.
   const string16 new_profile_name = L"New Profile Name";
   profile_info_cache_->SetNameOfProfileAtIndex(
       profile_info_cache_->GetIndexOfProfileWithPath(second_dest_path_),
       new_profile_name);
   RunPendingTasks();
   EXPECT_FALSE(ProfileShortcutExistsAtDefaultPath(second_profile_name_));
   ValidateProfileShortcut(new_profile_name);
 }
 
 TEST_F(ProfileShortcutManagerTest, DesktopShortcutsDeleteSecondToLast) {
-  ProfileShortcutManagerTest::SetupAndCreateTwoShortcuts();
+  SetupAndCreateTwoShortcuts();
 
   // Delete one shortcut.
   profile_info_cache_->DeleteProfileFromCache(second_dest_path_);
   RunPendingTasks();
   EXPECT_FALSE(ProfileShortcutExistsAtDefaultPath(second_profile_name_));
 
   // Verify that the profile name has been removed from the remaining shortcut.
   ValidateProfileShortcut(string16());
   // Verify that an additional shortcut, with the default profile's name does
   // not exist.
   EXPECT_FALSE(ProfileShortcutExistsAtDefaultPath(profile_name_));
 }
 
 TEST_F(ProfileShortcutManagerTest, DesktopShortcutsCreateSecond) {
-  ProfileShortcutManagerTest::SetupAndCreateTwoShortcuts();
+  SetupAndCreateTwoShortcuts();
 
   // Delete one shortcut.
   profile_info_cache_->DeleteProfileFromCache(second_dest_path_);
   RunPendingTasks();
 
   // Verify that a default shortcut exists (no profile name/avatar).
   ValidateProfileShortcut(string16());
   // Verify that an additional shortcut, with the default profile's name does
   // not exist.
   EXPECT_FALSE(ProfileShortcutExistsAtDefaultPath(profile_name_));
 
   // Create a second profile and shortcut.
   profile_info_cache_->AddProfileToCache(second_dest_path_,
                                          second_profile_name_, string16(), 0);
   profile_shortcut_manager_->CreateProfileShortcut(second_dest_path_);
   RunPendingTasks();
   ValidateProfileShortcut(second_profile_name_);
 
   // Verify that the original shortcut received the profile's name.
   ValidateProfileShortcut(profile_name_);
   // Verify that a default shortcut no longer exists.
   EXPECT_FALSE(ProfileShortcutExistsAtDefaultPath(string16()));
 }
 
 TEST_F(ProfileShortcutManagerTest, RenamedDesktopShortcuts) {
-  ProfileShortcutManagerTest::SetupAndCreateTwoShortcuts();
+  SetupAndCreateTwoShortcuts();
 
   const FilePath old_shortcut_path =
       GetDefaultShortcutPathForProfile(second_profile_name_);
   const FilePath new_shortcut_path =
       shortcuts_directory_.Append(L"MyChrome.lnk");
   ASSERT_TRUE(file_util::Move(old_shortcut_path, new_shortcut_path));
 
   // Ensure that a new shortcut does not get made if the old one was renamed.
   profile_shortcut_manager_->CreateProfileShortcut(second_dest_path_);
   RunPendingTasks();
   EXPECT_FALSE(ProfileShortcutExistsAtDefaultPath(second_profile_name_));
   EXPECT_TRUE(file_util::PathExists(new_shortcut_path));
 
   // Delete the renamed shortcut and try to create it again, which should work.
   ASSERT_TRUE(file_util::Delete(new_shortcut_path, false));
   EXPECT_FALSE(file_util::PathExists(new_shortcut_path));
   profile_shortcut_manager_->CreateProfileShortcut(second_dest_path_);
   RunPendingTasks();
   EXPECT_TRUE(ProfileShortcutExistsAtDefaultPath(second_profile_name_));
 }
 
 TEST_F(ProfileShortcutManagerTest, RenamedDesktopShortcutsGetDeleted) {
-  ProfileShortcutManagerTest::SetupAndCreateTwoShortcuts();
+  SetupAndCreateTwoShortcuts();
 
   const FilePath old_shortcut_path =
       GetDefaultShortcutPathForProfile(second_profile_name_);
   const FilePath new_shortcut_path =
       shortcuts_directory_.Append(L"MyChrome.lnk");
   // Make a copy of the shortcut.
   ASSERT_TRUE(file_util::CopyFile(old_shortcut_path, new_shortcut_path));
   EXPECT_TRUE(file_util::PathExists(old_shortcut_path));
   EXPECT_TRUE(file_util::PathExists(new_shortcut_path));
 
   // Also, copy the shortcut for the first user and ensure it gets preserved.
   const FilePath preserved_shortcut_path =
       shortcuts_directory_.Append(L"Preserved.lnk");
   ASSERT_TRUE(file_util::CopyFile(
       GetDefaultShortcutPathForProfile(profile_name_),
       preserved_shortcut_path));
   EXPECT_TRUE(file_util::PathExists(preserved_shortcut_path));
 
   // Delete the profile and ensure both shortcuts were also deleted.
   profile_info_cache_->DeleteProfileFromCache(second_dest_path_);
   RunPendingTasks();
   EXPECT_FALSE(file_util::PathExists(old_shortcut_path));
   EXPECT_FALSE(file_util::PathExists(new_shortcut_path));
   EXPECT_TRUE(file_util::PathExists(preserved_shortcut_path));
 }
 
 TEST_F(ProfileShortcutManagerTest, RenamedDesktopShortcutsAfterProfileRename) {
-  ProfileShortcutManagerTest::SetupAndCreateTwoShortcuts();
+  SetupAndCreateTwoShortcuts();
 
   const FilePath old_shortcut_path =
       GetDefaultShortcutPathForProfile(second_profile_name_);
   const FilePath new_shortcut_path =
       shortcuts_directory_.Append(L"MyChrome.lnk");
 
   // Make a copy of the shortcut.
   ASSERT_TRUE(file_util::CopyFile(old_shortcut_path, new_shortcut_path));
   EXPECT_TRUE(file_util::PathExists(old_shortcut_path));
   EXPECT_TRUE(file_util::PathExists(new_shortcut_path));
 
   // Now, rename the profile.
   const string16 new_profile_name = L"New profile";
   ASSERT_NE(second_profile_name_, new_profile_name);
   profile_info_cache_->SetNameOfProfileAtIndex(
       profile_info_cache_->GetIndexOfProfileWithPath(second_dest_path_),
       new_profile_name);
   RunPendingTasks();
 
   // The original shortcut should be renamed but the copied shortcut should
   // keep its name.
   EXPECT_FALSE(file_util::PathExists(old_shortcut_path));
   EXPECT_TRUE(file_util::PathExists(new_shortcut_path));
   const FilePath updated_shortcut_path =
       GetDefaultShortcutPathForProfile(new_profile_name);
   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));
+}