ProfileShortcutManagerWin: Don't create user level shortcut when system level one exists.

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/base_paths.h"
 #include "base/file_util.h"
 #include "base/location.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"
@@ skipping 10 matching lines @@
 
 using content::BrowserThread;
 
 class ProfileShortcutManagerTest : public testing::Test {
  protected:
   ProfileShortcutManagerTest()
       : distribution_(NULL),
         ui_thread_(BrowserThread::UI, &message_loop_),
         file_thread_(BrowserThread::FILE, &message_loop_),
         profile_shortcut_manager_(NULL),
-        profile_info_cache_(NULL),
-        fake_user_desktop_(base::DIR_USER_DESKTOP) {
+        profile_info_cache_(NULL) {
   }
 
   virtual void SetUp() OVERRIDE {
     CoInitialize(NULL);
     distribution_ = BrowserDistribution::GetDistribution();
     ASSERT_TRUE(distribution_ != NULL);
 
     TestingBrowserProcess* browser_process =
         TestingBrowserProcess::GetGlobal();
     profile_manager_.reset(new TestingProfileManager(browser_process));
     ASSERT_TRUE(profile_manager_->SetUp());
     profile_info_cache_ = profile_manager_->profile_info_cache();
     profile_shortcut_manager_.reset(
         ProfileShortcutManager::Create(profile_manager_->profile_manager()));
 
     ASSERT_TRUE(PathService::Get(base::FILE_EXE, &exe_path_));
+    fake_user_desktop_.reset(

[sail, 2013/01/14 21:45:19]

Are you moving this here to ensure that it runs once for each test? If so then I
don't think that's necessary. The class will be constructed each time the test
is run.

[Alexei Svitkine (slow), 2013/01/14 22:40:24]

Ah, that makes sense. Thanks for letting me know, I'll change it back.

[Alexei Svitkine (slow), 2013/01/15 22:03:02]

Done.

+        new base::ScopedPathOverride(base::DIR_USER_DESKTOP));
     ASSERT_TRUE(ShellUtil::GetShortcutPath(ShellUtil::SHORTCUT_LOCATION_DESKTOP,
                                            distribution_,
                                            ShellUtil::CURRENT_USER,
                                            &shortcuts_directory_));
-
+    fake_system_desktop_.reset(

[sail, 2013/01/14 21:45:19]

same here

[Alexei Svitkine (slow), 2013/01/15 22:03:02]

Done.

+        new base::ScopedPathOverride(base::DIR_COMMON_DESKTOP));
+    ASSERT_TRUE(ShellUtil::GetShortcutPath(ShellUtil::SHORTCUT_LOCATION_DESKTOP,
+                                           distribution_,
+                                           ShellUtil::SYSTEM_LEVEL,
+                                           &system_shortcuts_directory_));
     profile_1_name_ = L"My profile";
     profile_1_path_ = CreateProfileDirectory(profile_1_name_);
     profile_2_name_ = L"My profile 2";
     profile_2_path_ = CreateProfileDirectory(profile_2_name_);
     profile_3_name_ = L"My profile 3";
     profile_3_path_ = CreateProfileDirectory(profile_3_name_);
   }
 
   virtual void TearDown() OVERRIDE {
     message_loop_.RunUntilIdle();
@@ skipping 139 matching lines @@
     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)) << location.ToString();
     EXPECT_TRUE(file_util::PathExists(shortcut_path)) << location.ToString();
 
     return shortcut_path;
   }
 
+  FilePath CreateRegularSystemLevelShortcut(
+      const tracked_objects::Location& location) {
+    installer::Product product(distribution_);
+    ShellUtil::ShortcutProperties properties(ShellUtil::SYSTEM_LEVEL);
+    product.AddDefaultShortcutProperties(exe_path_, &properties);
+    EXPECT_TRUE(ShellUtil::CreateOrUpdateShortcut(
+        ShellUtil::SHORTCUT_LOCATION_DESKTOP, distribution_, properties,
+        ShellUtil::SHELL_SHORTCUT_CREATE_ALWAYS)) << location.ToString();
+    const FilePath system_level_shortcut_path =
+        system_shortcuts_directory_.Append(distribution_->GetAppShortCutName() +
+                                           installer::kLnkExt);
+    EXPECT_TRUE(file_util::PathExists(system_level_shortcut_path))
+        << location.ToString();
+    return system_level_shortcut_path;
+  }
+
   void RenameProfile(const tracked_objects::Location& location,
                      const FilePath& profile_path,
                      const string16& new_profile_name) {
     const size_t profile_index =
         profile_info_cache_->GetIndexOfProfileWithPath(profile_2_path_);
     ASSERT_NE(std::string::npos, profile_index);
     ASSERT_NE(profile_info_cache_->GetNameOfProfileAtIndex(profile_index),
               new_profile_name);
     profile_info_cache_->SetNameOfProfileAtIndex(profile_index,
                                                  new_profile_name);
     RunPendingTasks();
   }
 
   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_;
+  scoped_ptr<base::ScopedPathOverride> fake_user_desktop_;
   FilePath shortcuts_directory_;
+  scoped_ptr<base::ScopedPathOverride> fake_system_desktop_;
+  FilePath system_shortcuts_directory_;

[sail, 2013/01/14 21:45:19]

Ideally we shouldn't cache things in member variables. If you're doing this to
make the code easier to use then use utility methods instead.

I think the following member variables should be removed:
  distribution_
  profile_info_cache_
  exe_path_
  shortcuts_directory_
  system_shortcuts_directory_

This also make it easier to understand how the class works.

[Alexei Svitkine (slow), 2013/01/14 22:40:24]

Things are cached in member variables to make the test cases' code more brief
and easier to read. Utility methods will not result in code that is as short.

If you still prefer I change it, let me do it in a separate CL, so we don't
introduce noise too this one, OK?

[sail, 2013/01/14 22:43:55]

It's not too bad, GetFakeUserDesktop(). That's only 2 more letters than the old
one.
Sounds good.

   string16 profile_1_name_;
   FilePath profile_1_path_;
   string16 profile_2_name_;
   FilePath profile_2_path_;
   string16 profile_3_name_;
   FilePath profile_3_path_;
 };
 
 TEST_F(ProfileShortcutManagerTest, ShortcutFilename) {
   const string16 kProfileName = L"Harry";
@@ skipping 370 matching lines @@
 
   // Delete the shortcut and check that the function returns false.
   const FilePath profile_2_shortcut_path =
       GetDefaultShortcutPathForProfile(profile_2_name_);
   ASSERT_TRUE(file_util::Delete(profile_2_shortcut_path, false));
   EXPECT_FALSE(file_util::PathExists(profile_2_shortcut_path));
   profile_shortcut_manager_->HasProfileShortcuts(profile_2_path_, callback);
   RunPendingTasks();
   EXPECT_FALSE(result.has_shortcuts);
 }
+
+TEST_F(ProfileShortcutManagerTest,
+       ShortcutNotCreatedWhenSystemLevelShortcutExists) {
+  const FilePath system_level_shortcut_path =
+      CreateRegularSystemLevelShortcut(FROM_HERE);
+
+  // Create the initial profile.
+  profile_info_cache_->AddProfileToCache(profile_1_path_, profile_1_name_,
+                                         string16(), 0, false);
+  RunPendingTasks();
+  ASSERT_EQ(1U, profile_info_cache_->GetNumberOfProfiles());
+
+  // Ensure system level continues to exist and user level was not created.
+  EXPECT_TRUE(file_util::PathExists(system_level_shortcut_path));
+  EXPECT_FALSE(file_util::PathExists(
+                   GetDefaultShortcutPathForProfile(string16())));
+}
+
+TEST_F(ProfileShortcutManagerTest,
+       DeleteSecondToLastProfileWithoutShortcutWhenSystemLevelShortcutExists) {
+  SetupAndCreateTwoShortcuts(FROM_HERE);
+
+  const FilePath profile_1_shortcut_path =
+      GetDefaultShortcutPathForProfile(profile_1_name_);
+  const FilePath profile_2_shortcut_path =
+      GetDefaultShortcutPathForProfile(profile_2_name_);
+
+  // Delete the shortcut for the first profile, but keep the one for the 2nd.

[gab, 2013/01/14 21:49:30]

Why is it necessary to delete shortcut 1 first?

Isn't this testing the case where you have two profile shortcuts, delete a
profile, and thus the last profile shortcut becomes a regular shortcut, but it
should simply get deleted if there is a system-level already present because
another regular shortcut is redundant?

[Alexei Svitkine (slow), 2013/01/14 22:40:24]

These two tests are based on the existing tests
DeleteSecondToLastProfileWithoutShortcut and
DeleteSecondToLastProfileWithShortcut.

In the new tests I'm adding, the profile deletion happens in the presence of a
system level shortcut, so the expected result is different. If there is no
system level shortcut, then a user-level one should be created. If there is, it
shouldn't be (since otherwise you'd be getting two non-profile shortcuts).

Why both cases? It's necessary to exercise two separate codepaths in the
implementation.

In the case when you're deleting the 2nd-to-last profile without a shortcut,
then the creation of the non-profile shortcut happens as a result of
RenameChromeDesktopShortcutForProfile() for the other profile.

In the case when you're deleting the 2nd-to-last profile that does have a
shortcut, the creation of the non-profile shortcut happens via
DeleteDesktopShortcutsAndIconFile().

The two test cases are necessary to ensure both code paths are covered.

Now, strictly speaking, the
DeleteSecondToLastProfileWithoutShortcutWhenSystemLevelShortcutExists test does
not need to delete the shortcut itself to achieve coverage for the code path its
testing, but I kept the new tests similar to the ones they're based on. I can
simplify if you prefer.

If you wish, I can simplify them as much as possible, by basically removing all
the ASSERTs/EXPECTs for the other conditions (e.g. profile shortcuts existing or
not existing), and just leave the check for the user level regular shortcut not
being created.

[gab, 2013/01/14 22:49:55]

Right that sgtm.
Right, I would prefer not manually deleting the shortcut, the deletion of the
profile should do that anyways; perhaps move
file_util::PathExists(profile_1_shortcut_path)); up right after
RunPendingTasks(); to make it clear that this one goes away at that point.

Deleting profile 1 when its shortcut was manually deleted and expecting the same
result is also an interesting test case from a black box p.o.v. (i.e., perhaps
its the same codepath for now, but its definitely not the same scenario).
I like that there be EXPECTs around as many methods as possible, I'm just not a
fan of doing the extra  unnecessary deletion when this is not the core of this
test case.

+  ASSERT_TRUE(file_util::Delete(profile_1_shortcut_path, false));
+  ASSERT_FALSE(file_util::PathExists(profile_1_shortcut_path));
+  ASSERT_TRUE(file_util::PathExists(profile_2_shortcut_path));
+
+  const FilePath system_level_shortcut_path =
+      CreateRegularSystemLevelShortcut(FROM_HERE);
+
+  // Delete the profile that doesn't have a shortcut.
+  profile_info_cache_->DeleteProfileFromCache(profile_1_path_);
+  RunPendingTasks();
+
+  // Verify that only the system level shortcut still exists.
+  EXPECT_TRUE(file_util::PathExists(system_level_shortcut_path));
+  EXPECT_FALSE(file_util::PathExists(
+                   GetDefaultShortcutPathForProfile(string16())));
+  EXPECT_FALSE(file_util::PathExists(profile_1_shortcut_path));
+  EXPECT_FALSE(file_util::PathExists(profile_2_shortcut_path));
+}
+
+TEST_F(ProfileShortcutManagerTest,
+       DeleteSecondToLastProfileWithShortcutWhenSystemShortcutLevelExists) {
+  SetupAndCreateTwoShortcuts(FROM_HERE);
+
+  const FilePath profile_1_shortcut_path =
+      GetDefaultShortcutPathForProfile(profile_1_name_);
+  const FilePath profile_2_shortcut_path =
+      GetDefaultShortcutPathForProfile(profile_2_name_);
+
+  // Delete the shortcut for the first profile, but keep the one for the 2nd.
+  ASSERT_TRUE(file_util::Delete(profile_1_shortcut_path, false));

[gab, 2013/01/14 21:49:30]

Same comment about why deleting shortcut 1? It probably makes sense to make a
test case that tests specifically for that (i.e., that the desired behavior is
still respected if shortcut 1 has already been deleted), but I don't think this
is the core of this specific test case.

+  ASSERT_FALSE(file_util::PathExists(profile_1_shortcut_path));
+  ASSERT_TRUE(file_util::PathExists(profile_2_shortcut_path));
+
+  const FilePath system_level_shortcut_path =
+      CreateRegularSystemLevelShortcut(FROM_HERE);
+
+  // Delete the profile that has a shortcut.
+  profile_info_cache_->DeleteProfileFromCache(profile_2_path_);
+  RunPendingTasks();
+
+  // Verify that only the system level shortcut still exists.
+  EXPECT_TRUE(file_util::PathExists(system_level_shortcut_path));
+  EXPECT_FALSE(file_util::PathExists(
+                   GetDefaultShortcutPathForProfile(string16())));
+  EXPECT_FALSE(file_util::PathExists(profile_1_shortcut_path));
+  EXPECT_FALSE(file_util::PathExists(profile_2_shortcut_path));
+}