Refactor ProfileInfoCache in c/b/ui/app_list

chrome/browser/ui/app_list/app_list_view_delegate.cc

Index: chrome/browser/ui/app_list/app_list_view_delegate.cc
diff --git a/chrome/browser/ui/app_list/app_list_view_delegate.cc b/chrome/browser/ui/app_list/app_list_view_delegate.cc
index c26a1baf8b17c96543516b4ed8cb081af9b7f8f1..ee2c3c13b70d4583c231987084ed6b6a85baca63 100644
--- a/chrome/browser/ui/app_list/app_list_view_delegate.cc
+++ b/chrome/browser/ui/app_list/app_list_view_delegate.cc
@@ -6,8 +6,6 @@
 
 #include <stddef.h>
 
-#include <vector>
-
 #include "apps/custom_launcher_page_contents.h"
 #include "base/callback.h"
 #include "base/command_line.h"
@@ -20,7 +18,8 @@
 #include "chrome/browser/apps/scoped_keep_alive.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chrome_notification_types.h"
-#include "chrome/browser/profiles/profile_info_cache.h"
+#include "chrome/browser/profiles/profile_attributes_entry.h"
+#include "chrome/browser/profiles/profile_attributes_storage.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/search/hotword_service.h"
 #include "chrome/browser/search/hotword_service_factory.h"
@@ -107,16 +106,17 @@ void CreateShortcutInWebAppDir(
 }
 #endif
 
-void PopulateUsers(const ProfileInfoCache& profile_info,
-                   const base::FilePath& active_profile_path,
+void PopulateUsers(const base::FilePath& active_profile_path,
                    app_list::AppListViewDelegate::Users* users) {
   users->clear();
-  const size_t count = profile_info.GetNumberOfProfiles();
-  for (size_t i = 0; i < count; ++i) {
+  std::vector<ProfileAttributesEntry*> entries = g_browser_process->

[lwchkg, 2016/01/27 22:37:19]

const possible. Will update in the next patch.

+      profile_manager()->GetProfileAttributesStorage().
+      GetAllProfilesAttributes();

[lwchkg, 2016/01/27 22:37:19]

Sorting is needed. (Anthony, I need something like your controversial LessThan.)

Anyway, I think that all calls to GetAllProfilesAttributes() needs the items to
be sorted in the same way, so it should be a good idea to do the sorting inside
that function. (If we somehow need the entries unsorted, we can also make
another function to get the entries that way.)

About the sorting algorithm: the current algorithm in ProfileInfoCache is
incorrect. Sorting of names should be locale aware.

(See
https://code.google.com/p/chromium/codesearch#chromium/src/components/bookmar...
for an example.)

If profile names are equal, then profile paths are best compared by length of
string and then string contents, so we have "Profile 11" > "Profile 10" >
"Profile 9", etc.

WDYT?

[anthonyvd, 2016/01/29 15:16:34]

I think you're right that all the places that need them sorted do need them
sorted the same way, although it's not impossible that some caller doesn't care
about the order. Part of the goals of the refactor is to 1) remove the sorting
cost when it's not needed and 2) get out of the situation where callers have
this implicit contract with the PIC that the order is a certain way. Because of
this, I wouldn't sort in GetAllProfileAttributes().

I could see a case being made for adding a GetAllProfileAttributesSorted() or
something if the proper ordering logic is complex but I'm still not sold on
that.
Interesting, I don't know if we have a common comparator (ie: not in bookmarks)
that does something like this that we could use.
Most places I've seen only care about name order since it's what's displayed so
that sounds good but it could be more nuanced in some parts of the code.

[lwchkg, 2016/01/29 19:01:48]

I see. Now I do see why you make a LessThan in your CL. I'll make a similar
function.

Anyway, sorting strings in non-English languages is indeed very complex. In
Chromium our best bet is to sort the string using ICU functions. We need to hope
that ICU collation is already set up properly in Chromium. (I have doubts in it
though.)

[lwchkg, 2016/01/31 16:47:34]

I don't know of one here. Anyway, each comparator looks a little bit different
(the strings are compared the same way, but the secondary criteria are
different).
Not sure, and we gotta be careful in every callsite that use
GetAllProfileAttributes (maybe we should ask explicitly if you're not the
owner).

+  for (const auto& entry : entries) {
     app_list::AppListViewDelegate::User user;
-    user.name = profile_info.GetNameOfProfileAtIndex(i);
-    user.email = profile_info.GetUserNameOfProfileAtIndex(i);
-    user.profile_path = profile_info.GetPathOfProfileAtIndex(i);
+    user.name = entry->GetName();
+    user.email = entry->GetUserName();
+    user.profile_path = entry->GetPath();
     user.active = active_profile_path == user.profile_path;
     users->push_back(user);
   }
@@ -199,7 +199,7 @@ AppListViewDelegate::AppListViewDelegate(AppListControllerDelegate* controller)
     }
   }
 
-  profile_manager->GetProfileInfoCache().AddObserver(this);
+  profile_manager->GetProfileAttributesStorage().AddObserver(this);
   speech_ui_.reset(new app_list::SpeechUIModel);
 
 #if defined(GOOGLE_CHROME_BUILD)
@@ -225,8 +225,8 @@ AppListViewDelegate::~AppListViewDelegate() {
   // by a leaky singleton. Essential shutdown work must be done by observing
   // chrome::NOTIFICATION_APP_TERMINATING.
   SetProfile(NULL);
-  g_browser_process->profile_manager()->GetProfileInfoCache().RemoveObserver(
-      this);
+  g_browser_process->profile_manager()->GetProfileAttributesStorage().
+      RemoveObserver(this);
 
   SigninManagerFactory* factory = SigninManagerFactory::GetInstance();
   if (factory)
@@ -330,9 +330,7 @@ void AppListViewDelegate::SetUpProfileSwitcher() {
     return;
 
   // Populate the app list users.
-  PopulateUsers(g_browser_process->profile_manager()->GetProfileInfoCache(),
-                profile_->GetPath(),
-                &users_);
+  PopulateUsers(profile_->GetPath(), &users_);
 
   FOR_EACH_OBSERVER(
       app_list::AppListViewDelegateObserver, observers_, OnProfilesChanged());