Refactor ProfileInfoCache in c/b/ui/views

chrome/browser/ui/views/frame/browser_view.cc

Index: chrome/browser/ui/views/frame/browser_view.cc
diff --git a/chrome/browser/ui/views/frame/browser_view.cc b/chrome/browser/ui/views/frame/browser_view.cc
index 9938bf33eeb3c0f59c16d3cd3a492ef27014923a..6139dab07ea17423553dfda4c3e346d47c464caf 100644
--- a/chrome/browser/ui/views/frame/browser_view.cc
+++ b/chrome/browser/ui/views/frame/browser_view.cc
@@ -32,8 +32,9 @@
 #include "chrome/browser/native_window_notification_source.h"
 #include "chrome/browser/profiles/avatar_menu.h"
 #include "chrome/browser/profiles/profile.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_info_cache.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/profiles/profile_window.h"
 #include "chrome/browser/profiles/profiles_state.h"
@@ -642,10 +643,9 @@ bool BrowserView::ShouldShowAvatar() const {
   // Tests may not have a profile manager.
   if (!g_browser_process->profile_manager())
     return false;
-  ProfileInfoCache& cache =
-      g_browser_process->profile_manager()->GetProfileInfoCache();
-  if (cache.GetIndexOfProfileWithPath(browser_->profile()->GetPath()) ==
-      std::string::npos) {
+  ProfileAttributesEntry* entry;
+  if (!g_browser_process->profile_manager()->GetProfileAttributesStorage().
+      GetProfileAttributesWithPath(browser_->profile()->GetPath(), &entry)) {

[lwchkg, 2016/02/14 17:07:18]

anthonyvd@ and mlerman@: AFAIK profiles either have an entry in
ProfileAttributesStorage, or is off the record (i.e. being incognito and/or
being guest/system). Am I correct? If yes the entire check can be removed.

[lwchkg, 2016/02/24 18:07:27]

Forgot to follow-up my own comment. Oh no!
Anyway, anthonyvd@ and mlerman@: WDYT?

[Mike Lerman, 2016/02/24 18:11:36]

In line 629 we explicitly don't skip out GuestProfile, so we need a check here
that handles Guest and System.

[Mike Lerman, 2016/02/24 18:11:37]

Well, Guest and System end up coming down to this line, since line 629
explicitly doesn't include Guest (or System, ideally)

[lwchkg, 2016/02/24 18:43:18]

Well... lines 626-637 is for ChromeOS, and lines 639-652 handles other OSes
(should be Win/Linux, and we're discussing the equivalent MacOS code in the
other review).

AFAIK guest and system profiles are be handled at line 641, because guest and
system profiles are both off the record. Maybe we can add explicitly the check
of guest and system profiles at the same line, to make the code easier to read.

[Mike Lerman, 2016/02/24 20:44:22]

I think the current condition is exactly what we need. It accomplishes: "if the
profileStorage, the place where we have avatars, doesn't have this profile, then
don't display an avatar". I think writing a condition that reads "if we're,
among other things, the system profile, then always display an avatar". It's
logically equivalent but doesn't convey the right semantics.

[lwchkg, 2016/02/26 15:27:30]

Finally I've found why the code was there: return false for profile in the
process being asynchronously deleted. I've missed the whole point. (Fortunately
we have git blames and diffs, otherwise I will never know it.)

Here is the diff:
https://chromium.googlesource.com/chromium/src/+/9bbc8241cae7b253ce22864fbe0f...

BTW, do we have a more readable way to check for this "profile being deleted"
status?

[Mike Lerman, 2016/02/26 15:47:49]

There isn't one currently. It would be easy to implement a public static method
on ProfileDestroyer that checks to see if there's an entry in
pending_destroyers_ and returns true/false as appropriate.

[lwchkg, 2016/02/26 16:47:33]

Found it. There's a IsProfileMarkedForDeletion() in the anonymous namespace in
profile_manager.cc. Just making it a public method in ProfileManager should do.

     return false;
   }