Improve the ProfileInfoCache API.

chrome/browser/profiles/profile_info_cache.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 "chrome/browser/profiles/profile_info_cache.h"
 
 #include "base/bind.h"
 #include "base/files/file_util.h"
 #include "base/i18n/case_conversion.h"
 #include "base/logging.h"
@@ skipping 1220 matching lines @@
   std::vector<base::FilePath>::const_iterator it;
   for (it = profiles_to_rename.begin(); it != profiles_to_rename.end(); ++it) {
     size_t profile_index = GetIndexOfProfileWithPath(*it);
     SetProfileIsUsingDefaultNameAtIndex(profile_index, true);
     // This will assign a new "Person %d" type name and re-sort the cache.
     SetNameOfProfileAtIndex(profile_index, ChooseNameForNewProfile(
         GetAvatarIconIndexOfProfileAtIndex(profile_index)));
   }
 #endif
 }
+
+// ProfileMetadataStorage implementation

[noms (inactive), 2015/06/29 17:10:39]

nit: . at the end?

[Mike Lerman, 2015/06/29 19:14:37]

I didn't think we generally had leading comments like this, that state what
these few functions will do; we should just follow the implementation order in
the header file.

+void ProfileInfoCache::AddProfile(
+    const base::FilePath& profile_path,
+    const base::string16& name,
+    const std::string& gaia_id,
+    const base::string16& user_name,
+    size_t icon_index,
+    const std::string& supervised_user_id) {
+  AddProfileToCache(
+      profile_path, name, gaia_id, user_name, icon_index, supervised_user_id);
+}
+void ProfileInfoCache::DeleteProfile(const base::FilePath& profile_path) {

[Mike Lerman, 2015/06/29 19:14:37]

nit: newline

+  DeleteProfileFromCache(profile_path);
+}
+
+std::vector<ProfileMetadataStorage::ProfileMetadataEntry>
+ProfileInfoCache::GetAllProfilesMetadata() {

[noms (inactive), 2015/06/29 17:10:40]

Yeah, here metadata is even worse (see the other comment about why I don't like
the word). Here, I'd expect this to return some metadata common to all profiles,
not all of the data for all of the profiles.

How about just `GetAllProfiles()` ?

While you're here, you're probably also going to need a `GetSortedProfiles()`,
for things like the avatar menu.

[anthonyvd, 2015/06/30 21:24:52]

You're absolutely right. I changed "Metadata" to "Attributes" and this method to
"GetAllProfilesAttributes()" since "GetAllProfiles()" could be confusing
(Profile is another type).

As for GetSortedProfiles(), I was planning on leaving the responsibility of
sorting to the callers that need it. Since only very few callers require that,
we'll be able to avoid keeping the entries sorted in the ProfileInfoCache. It
also allows callers to sort the profiles in the order they want instead of
having just that one option. In short, I think this is the responsibility of the
caller that needs it, not the storage object. Happy to hear your thoughts :)

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

Ok, it makes sense to leave sorting to the callers. Avatar_menu can I'm sure do
that nicely :)

+  std::vector<ProfileMetadataStorage::ProfileMetadataEntry> ret;
+  for (size_t i = 0; i < GetNumberOfProfiles(); ++i) {
+    ProfileMetadataStorage::ProfileMetadataEntry entry;
+    entry.profile_index_ = i;
+    entry.profile_info_cache_ = this;
+    ret.push_back(entry);
+  }
+  return ret;
+}
+
+bool ProfileInfoCache::GetProfileMetadataWithPath(
+    const base::FilePath& path, ProfileMetadataEntry* entry) {

[noms (inactive), 2015/06/29 17:10:39]

Idea: Why not return the `entry` (or null if it doesn't exist)? The caller has
to check that `entry` isn't null anyway, so this isn't worse.

[Mike Lerman, 2015/06/29 19:14:37]

I think the advantage of Anthony's implementation is that it's much more likely
for future developers to write code that actually checks the return of
GetProfileMetadataWithPath. if the function returns the entry, a lot of people
will just write GetProfileMetataWithProfile(profile-GetPath())->GetAThing().

[anthonyvd, 2015/06/30 21:24:52]

Exactly. The main reason for crashes right now is that it's not clear enough
that getting profile info for a path can fail. Callers should be checking that
GetIndexForProfileWithPath doesn't return string::npos but they don't :(

I think returning a bool and using an out parameter makes it more obvious that
the operation can fail and must be checked.

+  if (GetNumberOfProfiles() == 0) {
+    return false;
+  }
+
+  size_t index = GetIndexOfProfileWithPath(path);
+  if (index == std::string::npos) {
+    return false;
+  }
+
+  entry->profile_index_ = index;

[noms (inactive), 2015/06/29 17:10:40]

Weeeeeeeell if it's a private member, you probably should be using a setter
here. :)

[anthonyvd, 2015/06/30 21:24:52]

I wanted those to be private with the ProfileInfoCache as a friend since they're
an implementation detail and that as-is, the ProfileInfoCache is the only one
that has business creating Entries.

The latest patchset changes this to use a private constructor, still with
ProfileInfoCache as a friend. This is still meh but a little better maybe? Let
me know if you can think of a better way :)

+  entry->profile_info_cache_ = this;
+  return true;
+}
+
+// ProfileMetadataEntry implementation
+base::string16 ProfileMetadataStorage::ProfileMetadataEntry::GetName() const {

[noms (inactive), 2015/06/29 17:10:39]

I think this should be in a different file, maybe. Some Impl of sorts? This is
giant, and the profile info cache is already giant. 1500 lines of getters and
setters make pandas super sad

[Mike Lerman, 2015/06/29 19:14:37]

I agree - this seems like the wrong place for this. All of the ProfileInfoCache
methods you're calling are public.

[anthonyvd, 2015/06/30 21:24:52]

Good point, which kind of ties in to the other comment about moving the Entry
class to a separate file.

I went ahead and did that, let me know what you guys think :)

+  return profile_info_cache_->GetNameOfProfileAtIndex(profile_index_);
+}
+
+base::string16 ProfileMetadataStorage::ProfileMetadataEntry::GetShortcutName()
+    const {
+  return profile_info_cache_->GetShortcutNameOfProfileAtIndex(profile_index_);
+}
+
+base::FilePath ProfileMetadataStorage::ProfileMetadataEntry::GetPath() const {
+  return profile_info_cache_->GetPathOfProfileAtIndex(profile_index_);
+}
+
+base::Time ProfileMetadataStorage::ProfileMetadataEntry::GetActiveTime() const {
+  return profile_info_cache_->GetProfileActiveTimeAtIndex(profile_index_);
+}
+
+base::string16 ProfileMetadataStorage::ProfileMetadataEntry::GetUserName()
+    const {
+  return profile_info_cache_->GetUserNameOfProfileAtIndex(profile_index_);
+}
+
+const gfx::Image&
+ProfileMetadataStorage::ProfileMetadataEntry::GetAvatarIcon() {
+  return profile_info_cache_->GetAvatarIconOfProfileAtIndex(profile_index_);
+}
+
+std::string
+ProfileMetadataStorage::ProfileMetadataEntry::GetLocalAuthCredentials() const {
+  return profile_info_cache_->GetLocalAuthCredentialsOfProfileAtIndex(
+      profile_index_);
+}
+
+std::string
+ProfileMetadataStorage::ProfileMetadataEntry::GetPasswordChangeDetectionToken()
+    const {
+  return profile_info_cache_->GetPasswordChangeDetectionTokenAtIndex(
+      profile_index_);
+}
+
+bool ProfileMetadataStorage::ProfileMetadataEntry::GetBackgroundStatus() const {
+  return profile_info_cache_->GetBackgroundStatusOfProfileAtIndex(
+      profile_index_);
+}
+
+base::string16 ProfileMetadataStorage::ProfileMetadataEntry::GetGAIAName()
+    const {
+  return profile_info_cache_->GetGAIANameOfProfileAtIndex(profile_index_);
+}
+
+base::string16 ProfileMetadataStorage::ProfileMetadataEntry::GetGAIAGivenName()
+    const {
+  return profile_info_cache_->GetGAIAGivenNameOfProfileAtIndex(profile_index_);
+}
+
+std::string ProfileMetadataStorage::ProfileMetadataEntry::GetGAIAId() const {
+  return profile_info_cache_->GetGAIAIdOfProfileAtIndex(profile_index_);
+}
+
+const gfx::Image* ProfileMetadataStorage::ProfileMetadataEntry::GetGAIAPicture()
+    const {
+  return profile_info_cache_->GetGAIAPictureOfProfileAtIndex(profile_index_);
+}
+
+bool ProfileMetadataStorage::ProfileMetadataEntry::IsUsingGAIAPicture()
+    const {
+  return profile_info_cache_->IsUsingGAIAPictureOfProfileAtIndex(
+      profile_index_);
+}
+
+bool ProfileMetadataStorage::ProfileMetadataEntry::IsSupervised() const {
+  return profile_info_cache_->ProfileIsSupervisedAtIndex(profile_index_);
+}
+
+bool ProfileMetadataStorage::ProfileMetadataEntry::IsChild() const {
+  return profile_info_cache_->ProfileIsChildAtIndex(profile_index_);
+}
+
+bool ProfileMetadataStorage::ProfileMetadataEntry::IsLegacySupervised()
+    const {
+  return profile_info_cache_->ProfileIsLegacySupervisedAtIndex(profile_index_);
+}
+
+bool ProfileMetadataStorage::ProfileMetadataEntry::IsOmitted() const {
+  return profile_info_cache_->IsOmittedProfileAtIndex(profile_index_);
+}
+
+bool ProfileMetadataStorage::ProfileMetadataEntry::IsSigninRequired() const {
+  return profile_info_cache_->ProfileIsSigninRequiredAtIndex(profile_index_);
+}
+
+std::string ProfileMetadataStorage::ProfileMetadataEntry::GetSupervisedUserId()
+    const {
+  return profile_info_cache_->GetSupervisedUserIdOfProfileAtIndex(
+      profile_index_);
+}
+
+bool ProfileMetadataStorage::ProfileMetadataEntry::IsEphemeral() const {
+  return profile_info_cache_->ProfileIsEphemeralAtIndex(profile_index_);
+}
+
+bool ProfileMetadataStorage::ProfileMetadataEntry::IsUsingDefaultName()
+    const {
+  return profile_info_cache_->ProfileIsUsingDefaultNameAtIndex(profile_index_);
+}
+
+bool ProfileMetadataStorage::ProfileMetadataEntry::IsAuthenticated() const {
+  return profile_info_cache_->ProfileIsAuthenticatedAtIndex(profile_index_);
+}
+
+bool ProfileMetadataStorage::ProfileMetadataEntry::IsUsingDefaultAvatar()
+    const {
+  return profile_info_cache_->ProfileIsUsingDefaultAvatarAtIndex(
+      profile_index_);
+}
+
+bool ProfileMetadataStorage::ProfileMetadataEntry::IsAuthError() const {
+  return profile_info_cache_->ProfileIsAuthErrorAtIndex(profile_index_);
+}
+
+size_t ProfileMetadataStorage::ProfileMetadataEntry::GetAvatarIconIndex()
+    const {
+  return profile_info_cache_->GetAvatarIconIndexOfProfileAtIndex(
+      profile_index_);
+}
+
+void ProfileMetadataStorage::ProfileMetadataEntry::SetName(
+    const base::string16& name) {
+  profile_info_cache_->SetNameOfProfileAtIndex(profile_index_, name);
+}
+
+void ProfileMetadataStorage::ProfileMetadataEntry::SetShortcutName(
+    const base::string16& name) {
+  profile_info_cache_->SetShortcutNameOfProfileAtIndex(profile_index_, name);
+}
+
+void ProfileMetadataStorage::ProfileMetadataEntry::SetIsOmitted(
+    bool is_omitted) {
+  profile_info_cache_->SetIsOmittedProfileAtIndex(profile_index_, is_omitted);
+}
+
+void ProfileMetadataStorage::ProfileMetadataEntry::SetSupervisedUserId(
+    const std::string& id) {
+  profile_info_cache_->SetSupervisedUserIdOfProfileAtIndex(profile_index_, id);
+}
+
+void ProfileMetadataStorage::ProfileMetadataEntry::SetLocalAuthCredentials(
+    const std::string& auth) {
+  profile_info_cache_->SetLocalAuthCredentialsOfProfileAtIndex(
+      profile_index_, auth);
+}
+
+void
+ProfileMetadataStorage::ProfileMetadataEntry::SetPasswordChangeDetectionToken(
+    const std::string& token) {
+  profile_info_cache_->SetPasswordChangeDetectionTokenAtIndex(
+      profile_index_, token);
+}
+
+void ProfileMetadataStorage::ProfileMetadataEntry::SetBackgroundStatus(
+    bool running_background_apps) {
+  profile_info_cache_->SetBackgroundStatusOfProfileAtIndex(
+      profile_index_, running_background_apps);
+}
+
+void ProfileMetadataStorage::ProfileMetadataEntry::SetGAIAName(
+    const base::string16& name) {
+  profile_info_cache_->SetGAIANameOfProfileAtIndex(profile_index_, name);
+}
+
+void ProfileMetadataStorage::ProfileMetadataEntry::SetGAIAGivenName(
+    const base::string16& name) {
+  profile_info_cache_->SetGAIAGivenNameOfProfileAtIndex(profile_index_, name);
+}
+
+void ProfileMetadataStorage::ProfileMetadataEntry::SetGAIAPicture(
+    const gfx::Image* image) {
+  profile_info_cache_->SetGAIAPictureOfProfileAtIndex(profile_index_, image);
+}
+
+void ProfileMetadataStorage::ProfileMetadataEntry::SetIsUsingGAIAPicture(
+    bool value) {
+  profile_info_cache_->SetIsUsingGAIAPictureOfProfileAtIndex(
+      profile_index_, value);
+}
+
+void ProfileMetadataStorage::ProfileMetadataEntry::SetIsSigninRequired(
+    bool value) {
+  profile_info_cache_->SetProfileSigninRequiredAtIndex(profile_index_, value);
+}
+
+void ProfileMetadataStorage::ProfileMetadataEntry::SetIsEphemeral(bool value) {
+  profile_info_cache_->SetProfileIsEphemeralAtIndex(profile_index_, value);
+}
+
+void ProfileMetadataStorage::ProfileMetadataEntry::SetIsUsingDefaultName(
+    bool value) {
+  profile_info_cache_->SetProfileIsUsingDefaultNameAtIndex(
+      profile_index_, value);
+}
+
+void ProfileMetadataStorage::ProfileMetadataEntry::SetIsUsingDefaultAvatar(
+    bool value) {
+  profile_info_cache_->SetProfileIsUsingDefaultAvatarAtIndex(
+      profile_index_, value);
+}
+
+void ProfileMetadataStorage::ProfileMetadataEntry::SetIsAuthError(bool value) {
+  profile_info_cache_->SetProfileIsAuthErrorAtIndex(profile_index_, value);
+}
+
+void ProfileMetadataStorage::ProfileMetadataEntry::SetAvatarIconIndex(
+    size_t icon_index) {
+  profile_info_cache_->SetAvatarIconOfProfileAtIndex(
+      profile_index_, icon_index);
+}
+
+void ProfileMetadataStorage::ProfileMetadataEntry::SetAuthInfo(
+    const std::string& gaia_id, const base::string16& user_name) {
+  profile_info_cache_->SetAuthInfoOfProfileAtIndex(
+      profile_index_, gaia_id, user_name);
+}