Refactor most c/b/profiles calls to ProfileInfoCache.

Refactor most c/b/profiles calls to ProfileInfoCache.

This CL refactors c/b/profiles to use the improved interface to the
ProfileInfoCache: ProfileAttributesStorage.

BUG=305720

[anthonyvd, 2015-07-24 14:47:59]

anthonyvd@chromium.org changed reviewers:
+ mlerman@chromium.org, noms@chromium.org

[anthonyvd, 2015-07-24 14:48:00]

Hi mlerman@, noms@!

This is the first follow up to the ProfileInfoCache refactor. This CL changes
all the ProfileInfoCache references in c/b/profiles (and some other places that
were consuming some profiles interfaces that slightly changed) to use
ProfileAttributesStorage.

Can you please take a look when you have a moment (this is only clean up work so
no hurry)? 

Thanks a ton!

[Mike Lerman, 2015-08-06 16:06:20]

Lots of work here! Looks really awesome!

https://codereview.chromium.org/1242793005/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/profiles/profile_list_chromeos.cc (right):

https://codereview.chromium.org/1242793005/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/profiles/profile_list_chromeos.cc:77: size_t
sorted_index =
Why not just keep an iterator i that you increment as you move through the loop?
Calling find() every time seems unnecessary (and CPU expensive)

https://codereview.chromium.org/1242793005/diff/60001/chrome/browser/profiles...
File chrome/browser/profiles/avatar_menu.cc (right):

https://codereview.chromium.org/1242793005/diff/60001/chrome/browser/profiles...
chrome/browser/profiles/avatar_menu.cc:65:
g_browser_process->profile_manager()->
The ctor was passed a ProfileAttributesStorage; why go through the
ProfileManager to get the reference?

https://codereview.chromium.org/1242793005/diff/60001/chrome/browser/profiles...
chrome/browser/profiles/avatar_menu.cc:158: profile_path);
Don't need to declare a FilePath, you can just inline the
GetItemAt().profile_path.

https://codereview.chromium.org/1242793005/diff/60001/chrome/browser/profiles...
File chrome/browser/profiles/avatar_menu.h (right):

https://codereview.chromium.org/1242793005/diff/60001/chrome/browser/profiles...
chrome/browser/profiles/avatar_menu.h:109: void SwitchToProfile(size_t index,
this used to be the index of the profile in the ProfileInfoCache. (or that's
what profile_list_desktop_browsertest heavily implies). Since we no longer have
that index, perhaps this should be changed to take a profile_path?

https://codereview.chromium.org/1242793005/diff/60001/chrome/browser/profiles...
File chrome/browser/profiles/avatar_menu_actions.h (right):

https://codereview.chromium.org/1242793005/diff/60001/chrome/browser/profiles...
chrome/browser/profiles/avatar_menu_actions.h:25: // Allows the user to edit the
profile at the given index in the cache.
change the comment, there's no index anymore.

https://codereview.chromium.org/1242793005/diff/60001/chrome/browser/profiles...
File chrome/browser/profiles/gaia_info_update_service_unittest.cc (right):

https://codereview.chromium.org/1242793005/diff/60001/chrome/browser/profiles...
chrome/browser/profiles/gaia_info_update_service_unittest.cc:77: return
GetCache();
Do we still need a GetCache() method?

https://codereview.chromium.org/1242793005/diff/60001/chrome/browser/profiles...
File chrome/browser/profiles/profile_attributes_entry.cc (right):

https://codereview.chromium.org/1242793005/diff/60001/chrome/browser/profiles...
chrome/browser/profiles/profile_attributes_entry.cc:234: return
profile_info_cache_->CacheKeyFromProfilePath(GetPath()).compare(
Why not just run compare on GetPath() (or some derivative)?

https://codereview.chromium.org/1242793005/diff/60001/chrome/browser/profiles...
File chrome/browser/profiles/profile_attributes_entry.h (right):

https://codereview.chromium.org/1242793005/diff/60001/chrome/browser/profiles...
chrome/browser/profiles/profile_attributes_entry.h:109: bool LessThan(const
ProfileAttributesEntry& other) const;
Add a comment here? It's unclear how we're comparing ProfileAttributeEntries.
(who needs this? Does this really belong here? I don't think "name" is necessary
the canonical way to compare profiles for all time, but maybe it is)

https://codereview.chromium.org/1242793005/diff/60001/chrome/browser/profiles...
File chrome/browser/profiles/profile_attributes_storage.h (right):

https://codereview.chromium.org/1242793005/diff/60001/chrome/browser/profiles...
chrome/browser/profiles/profile_attributes_storage.h:46: // Returns unique name
that can be assigned to a newly created profile.
insert "a" before "unique"

https://codereview.chromium.org/1242793005/diff/60001/chrome/browser/profiles...
File chrome/browser/profiles/profile_list_desktop.cc (left):

https://codereview.chromium.org/1242793005/diff/60001/chrome/browser/profiles...
chrome/browser/profiles/profile_list_desktop.cc:70: VLOG_IF(2,
(omitted_item_count_ > 1)) << omitted_item_count_
Can you check with treib@ or bernhard@ that we can remove the checks for
omitted_items safely?

https://codereview.chromium.org/1242793005/diff/60001/chrome/browser/profiles...
File chrome/browser/profiles/profile_list_desktop.cc (right):

https://codereview.chromium.org/1242793005/diff/60001/chrome/browser/profiles...
chrome/browser/profiles/profile_list_desktop.cc:44: return
first->LessThan(*second);
If this is the only reference to LessThan(), can we anonymously define a method
for comparing above?

https://codereview.chromium.org/1242793005/diff/60001/chrome/browser/profiles...
chrome/browser/profiles/profile_list_desktop.cc:52: gfx::Image icon =
entry->GetAvatarIcon();
You can just inline icon below.

https://codereview.chromium.org/1242793005/diff/60001/chrome/browser/profiles...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/1242793005/diff/60001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager.cc:1278: if (profile_in_storage)
Why do we need this caution now? How are we ever trying to delete a profile that
isn't in the cache? that wouldn't make sense

https://codereview.chromium.org/1242793005/diff/60001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager.cc:1344: NOTREACHED();
Can you add some descriptive comment at this NOTREACHED() (with <<)? And in what
circumstances would this ever occur?

If this fails, then CreateProfileAsync should probably call its callback with
CREATE_STATUS_LOCAL_FAIL or something similar, rather than continuing silently.

https://codereview.chromium.org/1242793005/diff/60001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager.cc:1348: if
(profile->GetPrefs()->GetBoolean(prefs::kForceEphemeralProfiles)) {
nit: no {}.

Can even just write new_entry->SetIsEphemeral(profile->GetPrefs()->GetBoolean())

https://codereview.chromium.org/1242793005/diff/60001/chrome/browser/profiles...
File chrome/browser/profiles/profile_manager.h (right):

https://codereview.chromium.org/1242793005/diff/60001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager.h:163: // Deprecated, use
GetProfileAttributesStorage() instead.
Perhaps put a comment on profile_info_cache.h as well, indicating something
similar?

https://codereview.chromium.org/1242793005/diff/60001/chrome/browser/profiles...
File chrome/browser/profiles/profile_metrics.cc (right):

https://codereview.chromium.org/1242793005/diff/60001/chrome/browser/profiles...
chrome/browser/profiles/profile_metrics.cc:159: for (ProfileAttributesEntry*
entry: entries) {
nit: space between entry and :

https://codereview.chromium.org/1242793005/diff/60001/chrome/browser/profiles...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

https://codereview.chromium.org/1242793005/diff/60001/chrome/browser/profiles...
chrome/browser/profiles/profile_shortcut_manager_win.cc:718: if
(!storage.GetProfileAttributesWithPath(profile_path, &entry)) {
Isn't this equivalent to DCHECK(storage.GetProfileAttr...)? Don't need the
conditional and the NOTREACHED?

https://codereview.chromium.org/1242793005/diff/60001/chrome/browser/profiles...
chrome/browser/profiles/profile_shortcut_manager_win.cc:764: DCHECK_EQ(1U,
entries.size());
We're only in this condition if NumberOfProfiles() == 1; I think entries.size()
== 1 is sufficiently obvious that we don't need to check it.

https://codereview.chromium.org/1242793005/diff/60001/chrome/browser/profiles...
chrome/browser/profiles/profile_shortcut_manager_win.cc:796:
ProfileAttributesEntry* current_entry;
I don't think you need all the extra checks. I think lines 795-808 can simply
be:

  std::vector<> entries = storage.GetAllProfilesAttributes();
  if (entries[0].GetPath() == profile_path)
    return entries[1].GetPath();
  return entries[0].GetPath();

That accurately reproduces what the function used to do.

https://codereview.chromium.org/1242793005/diff/60001/chrome/browser/profiles...
chrome/browser/profiles/profile_shortcut_manager_win.cc:804: DCHECK_EQ(2U,
entries.size());
you checked number of profiles == 2 at line 793, don't likely need to again.

https://codereview.chromium.org/1242793005/diff/60001/chrome/browser/profiles...
chrome/browser/profiles/profile_shortcut_manager_win.cc:822: if
(!storage.GetProfileAttributesWithPath(profile_path, &entry)) {
{ curlies }?

https://codereview.chromium.org/1242793005/diff/60001/chrome/browser/profiles...
File chrome/browser/profiles/profile_window.cc (right):

https://codereview.chromium.org/1242793005/diff/60001/chrome/browser/profiles...
chrome/browser/profiles/profile_window.cc:145: if (!profile->IsGuestSession() &&
entry->IsSigninRequired()) {
you don't need to verify IsGuestSession() twice (here and line 139)

https://codereview.chromium.org/1242793005/diff/60001/chrome/browser/profiles...
chrome/browser/profiles/profile_window.cc:223: page +=
base::IntToString(index_to_focus);
well... this is annoying. We probably shouldn't use an index (since
UserManagerScreenHandler::SendUserList() doesn't do any sorting). You'll need a
different unique identifier or hash-key (such as the profile directory perhaps,
although URL encoding an arbitrary file-system name is yucky). Or... sort
SendUserList.

https://codereview.chromium.org/1242793005/diff/60001/chrome/browser/profiles...
chrome/browser/profiles/profile_window.cc:433: for (ProfileAttributesEntry*
entry: entries) {
entry<space>: