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

chrome/browser/profiles/profile_manager_unittest.cc

Index: chrome/browser/profiles/profile_manager_unittest.cc
diff --git a/chrome/browser/profiles/profile_manager_unittest.cc b/chrome/browser/profiles/profile_manager_unittest.cc
index adaf53ac3db1cebc22dfab1971847e42cf02cdcf..37575cf01f708763e5be3c40fdebd231ba6f911c 100644
--- a/chrome/browser/profiles/profile_manager_unittest.cc
+++ b/chrome/browser/profiles/profile_manager_unittest.cc
@@ -51,6 +51,52 @@ 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;
@@ -450,6 +496,99 @@ TEST_F(ProfileManagerTest, GetLastUsedProfileAllowedByPolicy) {
       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) {