Improve the ProfileInfoCache API.

chrome/browser/profiles/profile_attributes_storage_unittest.cc

+// Copyright 2015 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 "base/strings/stringprintf.h"
+#include "base/strings/utf_string_conversions.h"
+#include "chrome/browser/profiles/profile_info_cache.h"
+#include "chrome/browser/profiles/profile_manager.h"
+#include "chrome/browser/supervised_user/supervised_user_constants.h"
+#include "chrome/test/base/testing_browser_process.h"
+#include "chrome/test/base/testing_profile_manager.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace {
+// The ProfileMetadataEntry accessors aren't just plain old accessors to local
+// members so they'll be tested. The following helpers will make the testing
+// code less verbose.
+#define TEST_ACCESSORS(entry, member, first_value, second_value) \
+    TestAccessors(&entry, \
+                  &decltype(entry)::Get ## member, \
+                  &decltype(entry)::Set ## member, \
+                  first_value, \
+                  second_value);
+
+#define TEST_STRING16_ACCESSORS(entry, member) \
+    TEST_ACCESSORS(entry, member, \
+        base::ASCIIToUTF16("first_" #member "_value"), \
+        base::ASCIIToUTF16("second_" #member "_value"));
+
+#define TEST_STRING_ACCESSORS(entry, member) \
+    TEST_ACCESSORS(entry, member, \
+        std::string("first_" #member "_value"), \
+        std::string("second_" #member "_value"));
+
+#define TEST_BOOL_ACCESSORS(entry, member) \
+TestAccessors(&entry, \
+              &decltype(entry)::member, \
+              &decltype(entry)::Set ## member, \
+              false, \
+              true);
+
+template<typename TValue, typename TGetter, typename TSetter>
+void TestAccessors(ProfileAttributesEntry* entry,
+    TGetter getter_func,
+    TSetter setter_func,
+    TValue first_value,
+    TValue second_value) {
+  (entry->*setter_func)(first_value);
+  EXPECT_EQ(first_value, (entry->*getter_func)());
+  (entry->*setter_func)(second_value);
+  EXPECT_EQ(second_value, (entry->*getter_func)());
+}
+}  // namespace
+
+class ProfileAttributesStorageTest : public testing::Test {
+ public:
+  ProfileAttributesStorageTest()
+      : testing_profile_manager_(TestingBrowserProcess::GetGlobal()) {}
+  ~ProfileAttributesStorageTest() override {}
+
+ protected:
+  void SetUp() override {
+    ASSERT_TRUE(testing_profile_manager_.SetUp());
+  }
+
+  void TearDown() override {
+  }
+
+  base::FilePath GetProfilePath(const std::string& base_name) {
+    return testing_profile_manager_.profile_manager()->user_data_dir().
+        AppendASCII(base_name);
+  }
+
+  ProfileAttributesStorage* storage() {
+    return testing_profile_manager_.profile_info_cache();
+  }
+
+  void AddTestingProfile() {
+    size_t number_of_profiles = storage()->GetNumberOfProfiles();
+
+    storage()->AddProfile(
+        GetProfilePath(
+            base::StringPrintf("testing_profile_path%lu", number_of_profiles)),
+        base::ASCIIToUTF16(
+            base::StringPrintf("testing_profile_name%lu", number_of_profiles)),
+        std::string(
+            base::StringPrintf("testing_profile_gaia%lu", number_of_profiles)),
+        base::ASCIIToUTF16(
+            base::StringPrintf("testing_profile_user%lu", number_of_profiles)),
+        number_of_profiles,
+        std::string(""));
+
+    EXPECT_EQ(number_of_profiles + 1, storage()->GetNumberOfProfiles());
+  }
+
+ private:
+  TestingProfileManager testing_profile_manager_;
+};
+
+TEST_F(ProfileAttributesStorageTest, ProfileNotFound) {
+  EXPECT_EQ(0U, storage()->GetNumberOfProfiles());
+
+  ProfileAttributesEntry entry;
+  ASSERT_FALSE(storage()->GetProfileAttributesWithPath(
+      GetProfilePath("testing_profile_path0"), &entry));
+
+  AddTestingProfile();
+  EXPECT_EQ(1U, storage()->GetNumberOfProfiles());
+
+  ASSERT_TRUE(storage()->GetProfileAttributesWithPath(
+      GetProfilePath("testing_profile_path0"), &entry));
+  ASSERT_FALSE(storage()->GetProfileAttributesWithPath(
+      GetProfilePath("testing_profile_path1"), &entry));
+}
+
+TEST_F(ProfileAttributesStorageTest, AddProfile) {
+  EXPECT_EQ(0U, storage()->GetNumberOfProfiles());
+
+  storage()->AddProfile(GetProfilePath("new_profile_path_1"),
+                        base::ASCIIToUTF16("new_profile_name_1"),
+                        std::string("new_profile_gaia_1"),
+                        base::ASCIIToUTF16("new_profile_username_1"),
+                        1,
+                        std::string(""));
+
+  EXPECT_EQ(1U, storage()->GetNumberOfProfiles());
+
+  ProfileAttributesEntry entry;
+  ASSERT_TRUE(storage()->GetProfileAttributesWithPath(
+      GetProfilePath("new_profile_path_1"), &entry));
+  EXPECT_EQ(base::ASCIIToUTF16("new_profile_name_1"), entry.GetName());
+}
+
+TEST_F(ProfileAttributesStorageTest, DeleteProfile) {
+  EXPECT_EQ(0U, storage()->GetNumberOfProfiles());
+
+  ProfileAttributesEntry entry;
+  ASSERT_FALSE(storage()->GetProfileAttributesWithPath(
+      GetProfilePath("testing_profile_path0"), &entry));
+
+  AddTestingProfile();
+  EXPECT_EQ(1U, storage()->GetNumberOfProfiles());
+
+  // Deleting an existing profile should make it un-retrievable.
+  storage()->DeleteProfile(GetProfilePath("testing_profile_path0"));
+  EXPECT_EQ(0U, storage()->GetNumberOfProfiles());
+
+  ASSERT_FALSE(storage()->GetProfileAttributesWithPath(
+      GetProfilePath("testing_profile_path1"), &entry));
+  ASSERT_FALSE(storage()->GetProfileAttributesWithPath(
+      GetProfilePath("testing_profile_path1"), &entry));
+}
+
+TEST_F(ProfileAttributesStorageTest, MultipleProfiles) {
+  EXPECT_EQ(0U, storage()->GetNumberOfProfiles());
+
+  for (size_t i = 0; i < 5; ++i) {
+    AddTestingProfile();
+    EXPECT_EQ(i + 1, storage()->GetNumberOfProfiles());
+    std::vector<ProfileAttributesEntry> entries =
+        storage()->GetAllProfilesAttributes();
+    EXPECT_EQ(i + 1, entries.size());
+  }
+
+  EXPECT_EQ(5U, storage()->GetNumberOfProfiles());
+
+  ProfileAttributesEntry entry;
+  ASSERT_TRUE(storage()->GetProfileAttributesWithPath(
+      GetProfilePath("testing_profile_path0"), &entry));
+  EXPECT_EQ(base::ASCIIToUTF16("testing_profile_name0"), entry.GetName());
+
+  storage()->DeleteProfile(GetProfilePath("testing_profile_path0"));
+  ASSERT_FALSE(storage()->GetProfileAttributesWithPath(
+      GetProfilePath("testing_profile_path0"), &entry));
+  EXPECT_EQ(4U, storage()->GetNumberOfProfiles());
+
+  std::vector<ProfileAttributesEntry> entries =
+      storage()->GetAllProfilesAttributes();
+  for (auto& entry: entries) {
+    EXPECT_NE(GetProfilePath("testing_profile_path0"), entry.GetPath());
+  }
+}
+
+TEST_F(ProfileAttributesStorageTest, InitialValues) {
+  AddTestingProfile();
+
+  ProfileAttributesEntry entry;
+  ASSERT_TRUE(storage()->GetProfileAttributesWithPath(
+      GetProfilePath("testing_profile_path0"), &entry));
+  EXPECT_EQ(GetProfilePath("testing_profile_path0"), entry.GetPath());
+  EXPECT_EQ(base::ASCIIToUTF16("testing_profile_name0"), entry.GetName());
+  EXPECT_EQ(std::string("testing_profile_gaia0"), entry.GetGAIAId());
+  EXPECT_EQ(base::ASCIIToUTF16("testing_profile_user0"), entry.GetUserName());
+  EXPECT_EQ(0U, entry.GetAvatarIconIndex());
+  EXPECT_EQ(std::string(""), entry.GetSupervisedUserId());
+}
+
+TEST_F(ProfileAttributesStorageTest, EntryAccessors) {
+  AddTestingProfile();
+
+  ProfileAttributesEntry entry;
+  ASSERT_TRUE(storage()->GetProfileAttributesWithPath(
+      GetProfilePath("testing_profile_path0"), &entry));
+
+  EXPECT_EQ(GetProfilePath("testing_profile_path0"), entry.GetPath());
+
+  TEST_STRING16_ACCESSORS(entry, Name);
+  TEST_STRING16_ACCESSORS(entry, ShortcutName);
+  TEST_STRING_ACCESSORS(entry, LocalAuthCredentials);
+  TEST_STRING_ACCESSORS(entry, PasswordChangeDetectionToken);
+  TEST_ACCESSORS(entry, BackgroundStatus, false, true);
+  TEST_STRING16_ACCESSORS(entry, GAIAName);
+  TEST_STRING16_ACCESSORS(entry, GAIAGivenName);
+  TEST_BOOL_ACCESSORS(entry, IsUsingGAIAPicture);
+  TEST_BOOL_ACCESSORS(entry, IsOmitted);
+  TEST_BOOL_ACCESSORS(entry, IsSigninRequired);
+  TEST_STRING_ACCESSORS(entry, SupervisedUserId);
+  TEST_BOOL_ACCESSORS(entry, IsEphemeral);
+  TEST_BOOL_ACCESSORS(entry, IsUsingDefaultName);
+  TEST_BOOL_ACCESSORS(entry, IsUsingDefaultAvatar);
+  TEST_BOOL_ACCESSORS(entry, IsAuthError);
+}
+
+TEST_F(ProfileAttributesStorageTest, AuthInfo) {
+  AddTestingProfile();
+
+  ProfileAttributesEntry entry;
+  ASSERT_TRUE(storage()->GetProfileAttributesWithPath(
+      GetProfilePath("testing_profile_path0"), &entry));
+
+  entry.SetAuthInfo("", base::string16());
+  ASSERT_FALSE(entry.IsAuthenticated());
+  EXPECT_EQ(base::string16(), entry.GetUserName());
+  EXPECT_EQ("", entry.GetGAIAId());
+
+  entry.SetAuthInfo("foo", base::ASCIIToUTF16("bar"));
+  ASSERT_TRUE(entry.IsAuthenticated());
+  EXPECT_EQ(base::ASCIIToUTF16("bar"), entry.GetUserName());
+  EXPECT_EQ("foo", entry.GetGAIAId());
+}
+
+TEST_F(ProfileAttributesStorageTest, SupervisedUsersAccessors) {
+  AddTestingProfile();
+
+  ProfileAttributesEntry entry;
+  ASSERT_TRUE(storage()->GetProfileAttributesWithPath(
+      GetProfilePath("testing_profile_path0"), &entry));
+
+  entry.SetSupervisedUserId("");
+  ASSERT_FALSE(entry.IsSupervised());
+  ASSERT_FALSE(entry.IsChild());
+  ASSERT_FALSE(entry.IsLegacySupervised());
+
+  entry.SetSupervisedUserId("some_supervised_user_id");
+  ASSERT_TRUE(entry.IsSupervised());
+  ASSERT_FALSE(entry.IsChild());
+  ASSERT_TRUE(entry.IsLegacySupervised());
+
+  entry.SetSupervisedUserId(supervised_users::kChildAccountSUID);
+  ASSERT_TRUE(entry.IsSupervised());
+  ASSERT_TRUE(entry.IsChild());
+  ASSERT_FALSE(entry.IsLegacySupervised());
+}

[Mike Lerman, 2015/07/02 18:01:30]

Can you add some tests around:
- I get a ProfileAttributes for A, delete a Profile B, then use my
ProfileAttributes from A - it still works?
- I get a ProfileAttributes for A, I rename B (which changes the indices), use
the ProfileAttributes from A - it still works?
- I get a ProfileAttributes for A, delete the Profile for A, try to use the
ProfileAttributes - does the system respond as it should?
- Other crazy scenarios...

[anthonyvd, 2015/07/07 21:05:32]

Done. Also testing deleting from the ProfileInfoCache's own method.
Done.
Well the contract with the ProfileAttributesStorage (as with the
ProfileInfoCache) is that this is undefined, for the same reasons caching the
index or the entry pointer is undefined.

If you have any suggestions w.r.t. improving this I'd be happy to implement them
but I can't think of a way other than doing the whole bool GetFoo(FooType* out);
pattern for every accessor. I think that's a little overkill.
I added tests around modification from other entry pointers and from the
ProfileInfoCache itself as well. Anything else in mind?

[Mike Lerman, 2015/07/08 18:26:34]

Undefined contract - understood. Makes sense.

I agree about using an out parameter for every accessor is overkill. Is it worth
adding a DCHECK? In the old code, it was impossible to get anything from the
ProfileInfoCache if the Profile had been deleted, because it gets removed. Now,
it is possible, but very very unpredictable.

Nothing else in mind :)

[anthonyvd, 2015/07/09 17:02:13]

Good point, I added a DCHECK in ProfileInfoAttributesEntry::profile_index().
It's called in every other method and it would return std::string::npos if the
profile had been deleted.

FWIW, I don't think the situation is less predictable than before the refactor
though. The only way to access a deleted profile now is to hold onto the Entry,
delete the profile, then access it again. If that happens, the Entry would be a
pointer to an invalidated (deleted) element of
ProfileInfoCache::profile_attributes_entries_ and dereferencing it would be
undefined.

Before, it would be less predictable. In the same situation, a caller would be
holding onto the index, deleting a profile, then trying to access it through the
index. This would either be out of range if the deleted profile was previously
the last one (again, undefined) or the index would be pointing to another
profile, potentially reading/modifying the wrong profile info.

If anything, it's probably safer after the change you suggested w.r.t. the
storage owning the entries and storing the path instead of the index.