Refactor ProfileInfoCache in c/b/ui/views

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

 // Copyright 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/views/frame/browser_view.h"
 
 #include <stdint.h>
 #include <algorithm>
 #include <utility>
 
@@ skipping 14 matching lines @@
 #include "chrome/browser/app_mode/app_mode_utils.h"
 #include "chrome/browser/bookmarks/bookmark_stats.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/extensions/extension_util.h"
 #include "chrome/browser/extensions/tab_helper.h"
 #include "chrome/browser/infobars/infobar_service.h"
 #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"
 #include "chrome/browser/search/search.h"
 #include "chrome/browser/sessions/tab_restore_service_factory.h"
 #include "chrome/browser/signin/chrome_signin_helper.h"
 #include "chrome/browser/themes/theme_properties.h"
 #include "chrome/browser/themes/theme_service.h"
 #include "chrome/browser/translate/chrome_translate_client.h"
 #include "chrome/browser/ui/bookmarks/bookmark_bar_constants.h"
@@ skipping 588 matching lines @@
   return chrome::MultiUserWindowManager::ShouldShowAvatar(
       browser_->window()->GetNativeWindow());
 #else
   if (!IsBrowserTypeNormal())
     return false;
   if (IsOffTheRecord())  // Desktop guest is incognito and needs avatar.
     return true;
   // 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;
   }
 
   return AvatarMenu::ShouldShowAvatarMenu();
 #endif
 }
 
 bool BrowserView::GetAccelerator(int cmd_id,
                                  ui::Accelerator* accelerator) const {
   // We retrieve the accelerator information for standard accelerators
@@ skipping 2078 matching lines @@
 }
 
 extensions::ActiveTabPermissionGranter*
 BrowserView::GetActiveTabPermissionGranter() {
   content::WebContents* web_contents = GetActiveWebContents();
   if (!web_contents)
     return nullptr;
   return extensions::TabHelper::FromWebContents(web_contents)
       ->active_tab_permission_granter();
 }