Refactor ProfileInfoCache in c/b/profiles

chrome/browser/profiles/profile_manager.cc

Index: chrome/browser/profiles/profile_manager.cc
diff --git a/chrome/browser/profiles/profile_manager.cc b/chrome/browser/profiles/profile_manager.cc
index a5de32353bfd31510a41fb5424bf5be5538b831b..91ab1bcae35f0a4f0ce4dd6983a9e368ece8bd56 100644
--- a/chrome/browser/profiles/profile_manager.cc
+++ b/chrome/browser/profiles/profile_manager.cc
@@ -37,6 +37,7 @@
 #include "chrome/browser/password_manager/password_store_factory.h"
 #include "chrome/browser/prefs/incognito_mode_prefs.h"
 #include "chrome/browser/profiles/bookmark_model_loaded_observer.h"
+#include "chrome/browser/profiles/profile_attributes_entry.h"
 #include "chrome/browser/profiles/profile_attributes_storage.h"
 #include "chrome/browser/profiles/profile_avatar_icon_util.h"
 #include "chrome/browser/profiles/profile_destroyer.h"
@@ -401,7 +402,7 @@ Profile* ProfileManager::GetProfile(const base::FilePath& profile_dir) {
 }
 
 size_t ProfileManager::GetNumberOfProfiles() {
-  return GetProfileInfoCache().GetNumberOfProfiles();
+  return GetProfileAttributesStorage().GetNumberOfProfiles();
 }
 
 void ProfileManager::CreateProfileAsync(
@@ -432,14 +433,12 @@ void ProfileManager::CreateProfileAsync(
   } else {
     // Initiate asynchronous creation process.
     info = RegisterProfile(CreateProfileAsyncHelper(profile_path, this), false);
-    ProfileInfoCache& cache = GetProfileInfoCache();
-    // Get the icon index from the user's icon url
     size_t icon_index;
     DCHECK(base::IsStringASCII(icon_url));
     if (profiles::IsDefaultAvatarIconUrl(icon_url, &icon_index)) {
       // add profile to cache with user selected name and avatar
-      cache.AddProfileToCache(profile_path, name, std::string(),
-                              base::string16(), icon_index, supervised_user_id);
+      GetProfileAttributesStorage().AddProfile(profile_path, name,
+          std::string(), base::string16(), icon_index, supervised_user_id);
     }
 
     if (!supervised_user_id.empty()) {
@@ -686,17 +685,18 @@ void ProfileManager::ScheduleProfileForDeletion(
     service->CancelDownloads();
   }
 
-  ProfileInfoCache& cache = GetProfileInfoCache();
-
+  ProfileAttributesStorage& storage = GetProfileAttributesStorage();
   // If we're deleting the last (non-legacy-supervised) profile, then create a
   // new profile in its place.
   base::FilePath last_non_supervised_profile_path;
-  for (size_t i = 0; i < cache.GetNumberOfProfiles(); ++i) {
-    base::FilePath cur_path = cache.GetPathOfProfileAtIndex(i);
+  std::vector<ProfileAttributesEntry*> entries =
+      storage.GetAllProfilesAttributes();
+  for (ProfileAttributesEntry* entry : entries) {

[Mike Lerman, 2016/03/18 17:10:44]

why assign storage.GetAllProfilesAttribures() to a temporary variable, why not
iterate over it directly?
for (ProfileAttributesEntry* entry : storage.GetAllProfilesAttributes())

[lwchkg, 2016/03/19 18:48:46]

Did think of this, because in other places the code is
g_browser_process->GetProfileManager()->GetProfileAttributesStorage().
    GetAllProfilesAttributes();
which is too long to be included in the for loop.

Sounds good anyway.

[Mike Lerman, 2016/03/30 01:18:10]

And really, I'm sure the compiler optimizes this anyways.
I'm fine with either one.

[lwchkg, 2016/03/31 19:32:26]

Stupid typo. Should be "Didn't think of this" (i.e. the opposite) instead.

+    base::FilePath cur_path = entry->GetPath();
     // Make sure that this profile is not pending deletion, and is not
     // legacy-supervised.
     if (cur_path != profile_dir &&
-        !cache.ProfileIsLegacySupervisedAtIndex(i) &&
+        !entry->IsLegacySupervised() &&
         !IsProfileMarkedForDeletion(cur_path)) {
       last_non_supervised_profile_path = cur_path;
       break;
@@ -710,7 +710,7 @@ void ProfileManager::ScheduleProfileForDeletion(
 #if !defined(OS_CHROMEOS) && !defined(OS_ANDROID)
     int avatar_index = profiles::GetPlaceholderAvatarIndex();
     new_avatar_url = profiles::GetDefaultAvatarIconUrl(avatar_index);
-    new_profile_name = cache.ChooseNameForNewProfile(avatar_index);
+    new_profile_name = storage.ChooseNameForNewProfile(avatar_index);
 #endif
 
     base::FilePath new_path(GenerateNextProfileDirectoryPath());
@@ -764,14 +764,14 @@ void ProfileManager::AutoloadProfiles() {
     return;
   }
 
-  ProfileInfoCache& cache = GetProfileInfoCache();
-  size_t number_of_profiles = cache.GetNumberOfProfiles();
-  for (size_t p = 0; p < number_of_profiles; ++p) {
-    if (cache.GetBackgroundStatusOfProfileAtIndex(p)) {
+  std::vector<ProfileAttributesEntry*> entries =
+      GetProfileAttributesStorage().GetAllProfilesAttributes();
+  for (ProfileAttributesEntry* entry : entries) {
+    if (entry->GetBackgroundStatus()) {
       // If status is true, that profile is running background apps. By calling
       // GetProfile, we automatically cause the profile to be loaded which will
       // register it with the BackgroundModeManager.
-      GetProfile(cache.GetPathOfProfileAtIndex(p));
+      GetProfile(entry->GetPath());
     }
   }
 }
@@ -782,11 +782,12 @@ void ProfileManager::CleanUpEphemeralProfiles() {
   bool last_active_profile_deleted = false;
   base::FilePath new_profile_path;
   std::vector<base::FilePath> profiles_to_delete;
-  ProfileInfoCache& profile_cache = GetProfileInfoCache();
-  size_t profiles_count = profile_cache.GetNumberOfProfiles();
-  for (size_t i = 0; i < profiles_count; ++i) {
-    base::FilePath profile_path = profile_cache.GetPathOfProfileAtIndex(i);
-    if (profile_cache.ProfileIsEphemeralAtIndex(i)) {
+  ProfileAttributesStorage& storage = GetProfileAttributesStorage();
+  std::vector<ProfileAttributesEntry*> entries =
+      storage.GetAllProfilesAttributes();
+  for (ProfileAttributesEntry* entry : entries) {
+    base::FilePath profile_path = entry->GetPath();
+    if (entry->IsEphemeral()) {
       profiles_to_delete.push_back(profile_path);
       if (profile_path.BaseName().MaybeAsASCII() == last_used_profile)
         last_active_profile_deleted = true;
@@ -809,15 +810,15 @@ void ProfileManager::CleanUpEphemeralProfiles() {
     BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
                             base::Bind(&NukeProfileFromDisk, profile_path));
 
-    profile_cache.DeleteProfileFromCache(profile_path);
+    storage.RemoveProfile(profile_path);
   }
 }
 
 void ProfileManager::InitProfileUserPrefs(Profile* profile) {
   TRACE_EVENT0("browser", "ProfileManager::InitProfileUserPrefs");
-  ProfileInfoCache& cache = GetProfileInfoCache();
+  ProfileAttributesStorage& storage = GetProfileAttributesStorage();
 
-  if (profile->GetPath().DirName() != cache.GetUserDataDir())
+  if (profile->GetPath().DirName() != user_data_dir())

[lwchkg, 2016/03/14 15:49:10]

I want to change this code (and a few similar code) into DCHECK, but I'm not
sure if this is 100% safe.

Anyway, there is no discussion about this line in the related code review:
https://codereview.chromium.org/8771024/

[Mike Lerman, 2016/03/18 17:10:44]

Maybe this helps with testing?
It seems safe to leave as is.

[lwchkg, 2016/03/19 18:48:46]

Not sure, but very possible. I guess it *helped* testing. Too bad the author
didn't add a comment for us.

If we want to know, we can try adding LOG(WARNING) and run all tests.
Agree. And despite my wants to change, leaving the code as is is the only safe
option unless we have more evidence. (And this won't happen soon.)

[Mike Lerman, 2016/03/30 01:18:10]

We can add a UserCount and then a Googler can go and check if we ever actually
hit this case.

[lwchkg, 2016/03/31 19:32:26]

Sorry... what is a UserCount?

[Mike Lerman, 2016/04/04 15:45:31]

It's a way to gather statistics from the Chrome userbase (for those who opt in).
The way to do so is with a call to UMA_HISTOGRAM_BOOLEAN(). You can set up a new
histogram in histograms.xml. You can verify locally by looking at
chrome://histograms and seeing what Chrome's taking note of (note: it always
tracks locally whether you've opted in or not, and uploads to Google only if
you've opted in)

     return;
 
   size_t avatar_index;
@@ -827,29 +828,28 @@ void ProfileManager::InitProfileUserPrefs(Profile* profile) {
     profile_name = l10n_util::GetStringUTF8(IDS_PROFILES_GUEST_PROFILE_NAME);
     avatar_index = 0;
   } else {
-    size_t profile_cache_index =
-        cache.GetIndexOfProfileWithPath(profile->GetPath());
-    // If the cache has an entry for this profile, use the cache data.
-    if (profile_cache_index != std::string::npos) {
-      avatar_index =
-          cache.GetAvatarIconIndexOfProfileAtIndex(profile_cache_index);
-      profile_name =
-          base::UTF16ToUTF8(cache.GetNameOfProfileAtIndex(profile_cache_index));
-      supervised_user_id =
-          cache.GetSupervisedUserIdOfProfileAtIndex(profile_cache_index);
+    ProfileAttributesEntry* entry;
+    bool has_entry = storage.GetProfileAttributesWithPath(profile->GetPath(),
+                                                          &entry);
+    // If the profile attributes storage has an entry for this profile, use the
+    // data in the profile attributes storage.
+    if (has_entry) {
+      avatar_index = entry->GetAvatarIconIndex();
+      profile_name = base::UTF16ToUTF8(entry->GetName());
+      supervised_user_id = entry->GetSupervisedUserId();
     } else if (profile->GetPath() ==
-               profiles::GetDefaultProfileDir(cache.GetUserDataDir())) {
+                   profiles::GetDefaultProfileDir(user_data_dir())) {
       avatar_index = profiles::GetPlaceholderAvatarIndex();
 #if !defined(OS_CHROMEOS) && !defined(OS_ANDROID)
       profile_name =
-          base::UTF16ToUTF8(cache.ChooseNameForNewProfile(avatar_index));
+          base::UTF16ToUTF8(storage.ChooseNameForNewProfile(avatar_index));
 #else
       profile_name = l10n_util::GetStringUTF8(IDS_DEFAULT_PROFILE_NAME);
 #endif
     } else {
-      avatar_index = cache.ChooseAvatarIconIndexForNewProfile();
+      avatar_index = storage.ChooseAvatarIconIndexForNewProfile();
       profile_name =
-          base::UTF16ToUTF8(cache.ChooseNameForNewProfile(avatar_index));
+          base::UTF16ToUTF8(storage.ChooseNameForNewProfile(avatar_index));
     }
   }
 
@@ -879,12 +879,12 @@ void ProfileManager::InitProfileUserPrefs(Profile* profile) {
 }
 
 void ProfileManager::RegisterTestingProfile(Profile* profile,
-                                            bool add_to_cache,
+                                            bool add_to_storage,
                                             bool start_deferred_task_runners) {
   RegisterProfile(profile, true);
-  if (add_to_cache) {
+  if (add_to_storage) {
     InitProfileUserPrefs(profile);
-    AddProfileToCache(profile);
+    AddProfileToStorage(profile);
   }
   if (start_deferred_task_runners) {
     StartupTaskRunnerServiceFactory::GetForProfile(profile)->
@@ -1056,7 +1056,7 @@ void ProfileManager::DoFinalInit(Profile* profile, bool go_off_the_record) {
   TRACK_SCOPED_REGION("Startup", "ProfileManager::DoFinalInit");
 
   DoFinalInitForServices(profile, go_off_the_record);
-  AddProfileToCache(profile);
+  AddProfileToStorage(profile);
   DoFinalInitLogging(profile);
 
   ProfileMetrics::LogNumberOfProfiles(this);
@@ -1065,9 +1065,10 @@ void ProfileManager::DoFinalInit(Profile* profile, bool go_off_the_record) {
       content::Source<Profile>(profile),
       content::NotificationService::NoDetails());
 
-  // Record statistics to ProfileInfoCache if statistics were not recorded
-  // during shutdown, i.e. the last shutdown was a system shutdown or a crash.
-  if (!profile->IsGuestSession() && !profile->IsSystemProfile() &&
+  // Record statistics to ProfileAttributesStorage if statistics were not
+  // recorded during shutdown, i.e. the last shutdown was a system shutdown or a
+  // crash.
+    if (!profile->IsGuestSession() && !profile->IsSystemProfile() &&
       !profile->IsNewProfile() && !go_off_the_record &&
       profile->GetLastSessionExitType() != Profile::EXIT_NORMAL) {
     profiles::GatherProfileStatistics(
@@ -1081,8 +1082,6 @@ void ProfileManager::DoFinalInitForServices(Profile* profile,
   TRACK_SCOPED_REGION("Startup", "ProfileManager::DoFinalInitForServices");
 
 #if defined(ENABLE_EXTENSIONS)
-  ProfileInfoCache& cache = GetProfileInfoCache();
-
   // Ensure that the HostContentSettingsMap has been created before the
   // ExtensionSystem is initialized otherwise the ExtensionSystem will be
   // registered twice
@@ -1102,9 +1101,10 @@ void ProfileManager::DoFinalInitForServices(Profile* profile,
   }
   // Set the block extensions bit on the ExtensionService. There likely are no
   // blockable extensions to block.
-  size_t profile_index = cache.GetIndexOfProfileWithPath(profile->GetPath());
-  if (profile_index != std::string::npos &&
-      cache.ProfileIsSigninRequiredAtIndex(profile_index)) {
+  ProfileAttributesEntry* entry;
+  bool has_entry = GetProfileAttributesStorage().
+                       GetProfileAttributesWithPath(profile->GetPath(), &entry);
+  if (has_entry && entry->IsSigninRequired()) {
     extensions::ExtensionSystem::Get(profile)
         ->extension_service()
         ->BlockAllExtensions();
@@ -1281,7 +1281,7 @@ void ProfileManager::FinishDeletingProfile(
   profiles::SetLastUsedProfile(
       new_active_profile_dir.BaseName().MaybeAsASCII());
 
-  ProfileInfoCache& cache = GetProfileInfoCache();
+  ProfileAttributesStorage& storage = GetProfileAttributesStorage();
   // TODO(sail): Due to bug 88586 we don't delete the profile instance. Once we
   // start deleting the profile instance we need to close background apps too.
   Profile* profile = GetProfileByPath(profile_dir);
@@ -1297,12 +1297,12 @@ void ProfileManager::FinishDeletingProfile(
     // By this point, all in-progress downloads for the profile being deleted
     // must have been canceled (crbug.com/336725).
     DCHECK(DownloadServiceFactory::GetForBrowserContext(profile)->
-           NonMaliciousDownloadCount() == 0);
+               NonMaliciousDownloadCount() == 0);
     BrowserList::CloseAllBrowsersWithProfile(profile);
 
     // Disable sync for doomed profile.
     if (ProfileSyncServiceFactory::GetInstance()->HasProfileSyncService(
-        profile)) {
+            profile)) {
       ProfileSyncService* sync_service =
           ProfileSyncServiceFactory::GetInstance()->GetForProfile(profile);
       if (sync_service->IsSyncRequested()) {
@@ -1316,8 +1316,10 @@ void ProfileManager::FinishDeletingProfile(
           profile)->RequestStop(ProfileSyncService::CLEAR_DATA);
     }
 
-    ProfileMetrics::LogProfileDelete(cache.ProfileIsAuthenticatedAtIndex(
-        cache.GetIndexOfProfileWithPath(profile_dir)));
+    ProfileAttributesEntry* entry;
+    bool has_entry = storage.GetProfileAttributesWithPath(profile_dir, &entry);
+    DCHECK(has_entry);
+    ProfileMetrics::LogProfileDelete(entry->IsAuthenticated());
     // Some platforms store passwords in keychains. They should be removed.
     scoped_refptr<password_manager::PasswordStore> password_store =
         PasswordStoreFactory::GetForProfile(
@@ -1340,7 +1342,7 @@ void ProfileManager::FinishDeletingProfile(
   // Queue even a profile that was nuked so it will be MarkedForDeletion and so
   // CreateProfileAsync can't create it.
   QueueProfileDirectoryForDeletion(profile_dir);
-  cache.DeleteProfileFromCache(profile_dir);
+  storage.RemoveProfile(profile_dir);
   ProfileMetrics::UpdateReportedProfilesStatistics(this);
 }
 #endif  // !defined(OS_ANDROID)
@@ -1361,12 +1363,11 @@ ProfileManager::ProfileInfo* ProfileManager::GetProfileInfoByPath(
   return (iter == profiles_info_.end()) ? NULL : iter->second.get();
 }
 
-void ProfileManager::AddProfileToCache(Profile* profile) {
+void ProfileManager::AddProfileToStorage(Profile* profile) {
   TRACE_EVENT0("browser", "ProfileManager::AddProfileToCache");
   if (profile->IsGuestSession() || profile->IsSystemProfile())
     return;
-  ProfileInfoCache& cache = GetProfileInfoCache();
-  if (profile->GetPath().DirName() != cache.GetUserDataDir())
+  if (profile->GetPath().DirName() != user_data_dir())
     return;
 
   SigninManagerBase* signin_manager =
@@ -1377,35 +1378,38 @@ void ProfileManager::AddProfileToCache(Profile* profile) {
       signin_manager->GetAuthenticatedAccountId());
   base::string16 username = base::UTF8ToUTF16(account_info.email);
 
-  size_t profile_index = cache.GetIndexOfProfileWithPath(profile->GetPath());
-  if (profile_index != std::string::npos) {
-    // The ProfileInfoCache's info must match the Signin Manager.
-    cache.SetAuthInfoOfProfileAtIndex(profile_index, account_info.gaia,
-                                      username);
-    return;
+  ProfileAttributesStorage& storage = GetProfileAttributesStorage();
+  {

[Mike Lerman, 2016/03/18 17:10:44]

you left a set of parens here

[lwchkg, 2016/03/19 18:48:46]

Actually I deliberately added the braces for scoping. I feel uncomfortable to
let entry and has_entry to survive the full function, because another set of
values is assigned to them in line 1409.

WDYT?

[Mike Lerman, 2016/03/30 01:18:10]

Sure, leave a comment for that please.

[lwchkg, 2016/03/31 19:32:26]

Acknowledged.

+    ProfileAttributesEntry* entry;
+    bool has_entry = storage.GetProfileAttributesWithPath(profile->GetPath(),
+                                                          &entry);
+    if (has_entry) {
+      // The ProfileAttributesStorage's info must match the Signin Manager.
+      entry->SetAuthInfo(account_info.gaia, username);
+      return;
+    }
   }
 
   // Profile name and avatar are set by InitProfileUserPrefs and stored in the
-  // profile. Use those values to setup the cache entry.
+  // profile. Use those values to setup the entry in profile attributes storage.
   base::string16 profile_name =
       base::UTF8ToUTF16(profile->GetPrefs()->GetString(prefs::kProfileName));
 
-  size_t icon_index = profile->GetPrefs()->GetInteger(
-      prefs::kProfileAvatarIndex);
+  size_t icon_index =
+      profile->GetPrefs()->GetInteger(prefs::kProfileAvatarIndex);
 
   std::string supervised_user_id =
       profile->GetPrefs()->GetString(prefs::kSupervisedUserId);
 
-  cache.AddProfileToCache(profile->GetPath(),
-                          profile_name,
-                          account_info.gaia,
-                          username,
-                          icon_index,
-                          supervised_user_id);
+  storage.AddProfile(profile->GetPath(), profile_name, account_info.gaia,
+                     username, icon_index, supervised_user_id);
 
   if (profile->GetPrefs()->GetBoolean(prefs::kForceEphemeralProfiles)) {
-    cache.SetProfileIsEphemeralAtIndex(
-        cache.GetIndexOfProfileWithPath(profile->GetPath()), true);
+    ProfileAttributesEntry* entry;
+    bool has_entry = storage.GetProfileAttributesWithPath(profile->GetPath(),
+                                                          &entry);
+    DCHECK(has_entry);
+    entry->SetIsEphemeral(true);
   }
 }
 
@@ -1457,19 +1461,17 @@ void ProfileManager::UpdateLastUser(Profile* last_active) {
     if (profile_path_base != GetLastUsedProfileName())
       profiles::SetLastUsedProfile(profile_path_base);
 
-    ProfileInfoCache& cache = GetProfileInfoCache();
-    size_t profile_index =
-        cache.GetIndexOfProfileWithPath(last_active->GetPath());
-    if (profile_index != std::string::npos) {
+    ProfileAttributesEntry* entry;
+    if (GetProfileAttributesStorage().
+            GetProfileAttributesWithPath(last_active->GetPath(), &entry)) {
 #if !defined(OS_CHROMEOS)
       // Incognito Profiles don't have ProfileKeyedServices.
       if (!last_active->IsOffTheRecord()) {
-        CrossDevicePromoFactory::GetForProfile(last_active)
-            ->MaybeBrowsingSessionStarted(
-                cache.GetProfileActiveTimeAtIndex(profile_index));
+        CrossDevicePromoFactory::GetForProfile(last_active)->
+            MaybeBrowsingSessionStarted(entry->GetActiveTime());
       }
 #endif
-      cache.SetProfileActiveTimeAtIndex(profile_index);
+      entry->SetActiveTimeToNow();
     }
   }
 }
@@ -1505,9 +1507,9 @@ void ProfileManager::BrowserListObserver::OnBrowserRemoved(
     g_browser_process->profile_manager()->ScheduleProfileForDeletion(
         path, ProfileManager::CreateCallback());
   } else if (!profile->IsSystemProfile()) {
-    // Gather statistics and store into ProfileInfoCache. For incognito profile
-    // we gather the statistics of its parent profile instead, because a window
-    // of the parent profile was open.
+    // Gather statistics and store into ProfileAttributesStorage. For incognito
+    // profile we gather the statistics of its parent profile instead, because a
+    // window of the parent profile was open.
     profiles::GatherProfileStatistics(
         original_profile, profiles::ProfileStatisticsCallback(), nullptr);
   }