[Mac] AppController needs to update its "last profile" pointer when the active profile is deleted

chrome/browser/profiles/profile_manager_unittest.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 <string>
 
 #include "base/command_line.h"
 #include "base/file_util.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/message_loop.h"
@@ skipping 33 matching lines @@
 #include "chrome/browser/chromeos/login/user_manager.h"
 #include "chrome/browser/chromeos/settings/cros_settings.h"
 #include "chrome/browser/chromeos/settings/device_settings_service.h"
 #include "chromeos/chromeos_switches.h"
 #endif
 
 using content::BrowserThread;
 
 namespace {
 
+void CheckLastUsedProfile(const base::FilePath& profile_path,
+                          const std::string& profile_name) {
+  EXPECT_EQ(
+      g_browser_process->profile_manager()->GetLastUsedProfile()->GetPath(),
+      profile_path);
+  EXPECT_EQ(
+      g_browser_process->local_state()->GetString(prefs::kProfileLastUsed),
+      profile_name);
+}
+
+// This class listens to the ProfileInfoCache for profile deletions
+// and then runs a callback, which checks that the correct
+// next active profile is set.
+class ProfileRemovalObserver : public ProfileInfoCacheObserver {
+ public:
+  ProfileRemovalObserver(
+      ProfileManager* profile_manager,
+      base::Callback<void(void)> cb)
+          : profile_manager_(profile_manager),
+            callback_(cb) {
+    profile_manager_->GetProfileInfoCache().AddObserver(this);
+  }
+
+  virtual ~ProfileRemovalObserver() {
+    profile_manager_->GetProfileInfoCache().RemoveObserver(this);
+  }
+ private:
+  // ProfileInfoCacheObserver implementation:
+  virtual void OnProfileAdded(const base::FilePath& profile_path) OVERRIDE {}
+  virtual void OnProfileWasRemoved(const base::FilePath& profile_path,
+                                   const string16& profile_name) OVERRIDE {
+    callback_.Run();
+  }
+  virtual void OnProfileWillBeRemoved(
+      const base::FilePath& profile_path) OVERRIDE {}
+  virtual void OnProfileNameChanged(
+      const base::FilePath& profile_path, const string16& old_profile_name)
+      OVERRIDE {}
+  virtual void OnProfileAvatarChanged(
+      const base::FilePath& profile_path) OVERRIDE {}
+
+  ProfileManager* profile_manager_;
+  base::Callback<void(void)> callback_;
+  DISALLOW_COPY_AND_ASSIGN(ProfileRemovalObserver);
+};
+
 // This global variable is used to check that value returned to different
 // observers is the same.
 Profile* g_created_profile;
 
 }  // namespace
 
 namespace testing {
 
 class ProfileManager : public ::ProfileManagerWithoutInit {
  public:
@@ skipping 379 matching lines @@
   EXPECT_FALSE(
       profile_manager->GetLastUsedProfileAllowedByPolicy()->IsOffTheRecord());
 
   // GetLastUsedProfileAllowedByPolicy() returns the incognito Profile when
   // incognito mode is forced.
   IncognitoModePrefs::SetAvailability(prefs, IncognitoModePrefs::FORCED);
   EXPECT_TRUE(
       profile_manager->GetLastUsedProfileAllowedByPolicy()->IsOffTheRecord());
 }
 
+TEST_F(ProfileManagerTest, ActiveProfileDeleted) {
+  ProfileManager* profile_manager = g_browser_process->profile_manager();
+  ASSERT_TRUE(profile_manager);
+  ProfileInfoCache& cache = profile_manager->GetProfileInfoCache();

[Alexei Svitkine (slow), 2013/06/12 21:42:23]

Either make it const or a pointer.

Style guide disallows non-const references. (And yes, I know ProfileInfoCache is
used like that from many places.)

[noms (inactive), 2013/06/13 20:56:25]

Done.

+
+  // Create and load two profiles.
+  base::FilePath dest_path1 =
+      temp_dir_.path().Append(FILE_PATH_LITERAL("New Profile 1"));
+  base::FilePath dest_path2 =
+      temp_dir_.path().Append(FILE_PATH_LITERAL("New Profile 2"));
+
+  MockObserver mock_observer;
+  EXPECT_CALL(mock_observer, OnProfileCreated(
+      testing::NotNull(), NotFail())).Times(testing::AtLeast(3));
+
+  profile_manager->CreateProfileAsync(dest_path1,
+      base::Bind(&MockObserver::OnProfileCreated,
+                 base::Unretained(&mock_observer)),
+                 string16(), string16(), false);

[Alexei Svitkine (slow), 2013/06/12 21:42:23]

Can you refactor this block into a helper function that just takes dest_path1
and mock_observer, so the test is more readable?

[noms (inactive), 2013/06/13 20:56:25]

Done.

+  profile_manager->CreateProfileAsync(dest_path2,
+      base::Bind(&MockObserver::OnProfileCreated,
+                 base::Unretained(&mock_observer)),
+                 string16(), string16(), false);
+
+  message_loop_.RunUntilIdle();
+  EXPECT_EQ(2u, profile_manager->GetLoadedProfiles().size());
+  EXPECT_EQ(2u, cache.GetNumberOfProfiles());
+
+  // Set the active profile.
+  PrefService* local_state = g_browser_process->local_state();
+  local_state->SetString(prefs::kProfileLastUsed,
+                         dest_path1.BaseName().MaybeAsASCII());
+
+  ProfileRemovalObserver observer(
+      profile_manager,
+      base::Bind(&CheckLastUsedProfile, dest_path2, "New Profile 2"));

[Alexei Svitkine (slow), 2013/06/12 21:42:23]

Do you need this class?

Can't you just do the calls below (schedule deletion, run until idle) and then
assert that the profile was deleted and then check last used profile?

[noms (inactive), 2013/06/13 20:56:25]

Done.

+
+  // Delete the active profile. The observer will check the next active profile
+  // is set correctly.
+  profile_manager->ScheduleProfileForDeletion(dest_path1,
+                                              ProfileManager::CreateCallback());
+  // Spin the message loop so that all the callbacks can finish running.
+  message_loop_.RunUntilIdle();
+}
+
+TEST_F(ProfileManagerTest, ActiveProfileDeletedNeedsToLoadNextProfile) {
+  ProfileManager* profile_manager = g_browser_process->profile_manager();
+  ASSERT_TRUE(profile_manager);
+  ProfileInfoCache& cache = profile_manager->GetProfileInfoCache();
+
+  // Create and load one profile, and just create a second profile.
+  base::FilePath dest_path1 =
+      temp_dir_.path().Append(FILE_PATH_LITERAL("New Profile 1"));
+  base::FilePath dest_path2 =
+      temp_dir_.path().Append(FILE_PATH_LITERAL("New Profile 2"));
+
+  MockObserver mock_observer;
+  EXPECT_CALL(mock_observer, OnProfileCreated(
+      testing::NotNull(), NotFail())).Times(testing::AtLeast(2));
+
+  profile_manager->CreateProfileAsync(dest_path1,
+      base::Bind(&MockObserver::OnProfileCreated,
+                 base::Unretained(&mock_observer)),
+                 string16(), string16(), false);
+
+  // Track the profile, but don't load it.
+  cache.AddProfileToCache(dest_path2, ASCIIToUTF16("New Profile 2"),
+                          string16(), 0, false);
+
+  message_loop_.RunUntilIdle();
+
+  EXPECT_EQ(1u, profile_manager->GetLoadedProfiles().size());
+  EXPECT_EQ(2u, cache.GetNumberOfProfiles());
+
+  // Set the active profile.
+  PrefService* local_state = g_browser_process->local_state();
+  local_state->SetString(prefs::kProfileLastUsed,
+                         dest_path1.BaseName().MaybeAsASCII());
+
+  ProfileRemovalObserver observer(
+      profile_manager,
+      base::Bind(&CheckLastUsedProfile, dest_path2, "New Profile 2"));
+
+  // Delete the active profile. This should switch and load the unloaded
+  // profile. The observer will check the next active profile
+  // is set correctly.
+  profile_manager->ScheduleProfileForDeletion(dest_path1,
+                                              ProfileManager::CreateCallback());
+
+  // Spin the message loop so that all the callbacks can finish running.
+  message_loop_.RunUntilIdle();
+}

[Alexei Svitkine (slow), 2013/06/12 21:42:23]

Can you also add a test that exercises the "re-try" behaviour?

i.e. when you delete a profile and the one you that would be loaded gets deleted
before it has a chance to load (so that the code re-tries).

+
 #if !defined(OS_ANDROID)
 // There's no Browser object on Android.
 TEST_F(ProfileManagerTest, LastOpenedProfiles) {
   base::FilePath dest_path1 = temp_dir_.path();
   dest_path1 = dest_path1.Append(FILE_PATH_LITERAL("New Profile 1"));
 
   base::FilePath dest_path2 = temp_dir_.path();
   dest_path2 = dest_path2.Append(FILE_PATH_LITERAL("New Profile 2"));
 
   ProfileManager* profile_manager = g_browser_process->profile_manager();
@@ skipping 170 matching lines @@
   browser2b.reset();
   last_opened_profiles = profile_manager->GetLastOpenedProfiles();
   ASSERT_EQ(1U, last_opened_profiles.size());
   EXPECT_EQ(profile1, last_opened_profiles[0]);
 
   browser1.reset();
   last_opened_profiles = profile_manager->GetLastOpenedProfiles();
   ASSERT_EQ(0U, last_opened_profiles.size());
 }
 #endif  // !defined(OS_ANDROID)