Refactor ProfileInfoCache in c/b/ui/app_list

chrome/browser/ui/app_list/app_list_view_delegate.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/ui/app_list/app_list_view_delegate.h"
 
 #include <stddef.h>
 
-#include <vector>
-
 #include "apps/custom_launcher_page_contents.h"
 #include "base/callback.h"
 #include "base/command_line.h"
 #include "base/files/file_path.h"
 #include "base/metrics/user_metrics.h"
 #include "base/prefs/pref_service.h"
 #include "base/profiler/scoped_tracker.h"
 #include "base/stl_util.h"
 #include "build/build_config.h"
 #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"
 #include "chrome/browser/search_engines/template_url_service_factory.h"
 #include "chrome/browser/ui/app_list/app_list_controller_delegate.h"
 #include "chrome/browser/ui/app_list/app_list_service.h"
 #include "chrome/browser/ui/app_list/app_list_syncable_service.h"
 #include "chrome/browser/ui/app_list/app_list_syncable_service_factory.h"
 #include "chrome/browser/ui/app_list/launcher_page_event_dispatcher.h"
 #include "chrome/browser/ui/app_list/search/search_controller_factory.h"
@@ skipping 66 matching lines @@
     base::Callback<void(const base::FilePath&)> callback,
     scoped_ptr<web_app::ShortcutInfo> info) {
   content::BrowserThread::PostTaskAndReplyWithResult(
       content::BrowserThread::FILE, FROM_HERE,
       base::Bind(web_app::CreateShortcutInWebAppDir, app_data_dir,
                  base::Passed(&info)),
       callback);
 }
 #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);
   }
 }
 
 // Gets a list of URLs of the custom launcher pages to show in the launcher.
 // Returns a URL for each installed launcher page. If --custom-launcher-page is
 // specified and valid, also includes that URL.
 void GetCustomLauncherPageUrls(content::BrowserContext* browser_context,
                                std::vector<GURL>* urls) {
@@ skipping 62 matching lines @@
        i != profiles.end();
        ++i) {
     SigninManagerBase* manager =
         SigninManagerFactory::GetForProfileIfExists(*i);
     if (manager) {
       DCHECK(!scoped_observer_.IsObserving(manager));
       scoped_observer_.Add(manager);
     }
   }
 
-  profile_manager->GetProfileInfoCache().AddObserver(this);
+  profile_manager->GetProfileAttributesStorage().AddObserver(this);
   speech_ui_.reset(new app_list::SpeechUIModel);
 
 #if defined(GOOGLE_CHROME_BUILD)
   gfx::ImageSkia* image;
   {
     // TODO(tapted): Remove ScopedTracker below once crbug.com/431326 is fixed.
     tracked_objects::ScopedTracker tracking_profile(
         FROM_HERE_WITH_EXPLICIT_FUNCTION("431326 GetImageSkiaNamed()"));
     image = ui::ResourceBundle::GetSharedInstance().GetImageSkiaNamed(
         IDR_APP_LIST_GOOGLE_LOGO_VOICE_SEARCH);
   }
 
   speech_ui_->set_logo(*image);
 #endif
 
   registrar_.Add(this,
                  chrome::NOTIFICATION_APP_TERMINATING,
                  content::NotificationService::AllSources());
 }
 
 AppListViewDelegate::~AppListViewDelegate() {
   // Note that the destructor is not always called. E.g. on Mac, this is owned
   // 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)
     factory->RemoveObserver(this);
 }
 
 void AppListViewDelegate::SetProfile(Profile* new_profile) {
   if (profile_ == new_profile)
     return;
 
@@ skipping 83 matching lines @@
   if (!profile_)
     return;
 
   // Don't populate the app list users if we are on the ash desktop.
   chrome::HostDesktopType desktop = chrome::GetHostDesktopTypeForNativeWindow(
       controller_->GetAppListWindow());
   if (desktop == chrome::HOST_DESKTOP_TYPE_ASH)
     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());
 }
 
 void AppListViewDelegate::SetUpCustomLauncherPages() {
   std::vector<GURL> custom_launcher_page_urls;
   GetCustomLauncherPageUrls(profile_, &custom_launcher_page_urls);
   if (custom_launcher_page_urls.empty())
     return;
@@ skipping 480 matching lines @@
 
       // SigninManagerFactory is not a leaky singleton (unlike this class), and
       // its destructor will check that it has no remaining observers.
       scoped_observer_.RemoveAll();
       SigninManagerFactory::GetInstance()->RemoveObserver(this);
       break;
     default:
       NOTREACHED();
   }
 }