Store profile avatar to preferences

Multi-profiles: Use profile cache for menu

With this change we now use the new ProfileInfoCache object to get information about profiles.

As per the multi-profiles UI discussion we also hide the avatar if the user only has one local profile.

BUG=
TEST=

[willchan no longer on Chromium, 2011-06-15 11:22:12]

I took a quick skim and sent a bunch of nits. I'm going to defer to Miranda
here. Some general comments:

I note you named this ProfileInfoCache. Do you intend to add more things here
than just avatars? I see that this maintains an index of profiles, which seems
parallel to ProfileManager's index. Offhand (I haven't really examined the
code), I'd have thought that it'd be nicer to maintain a single vector. Perhaps
this should just be attached to a single Profile instance instead? I have no
idea. I'm going to shut up and let you and Miranda figure it out :)

http://codereview.chromium.org/7155015/diff/1/chrome/browser/profiles/profile...
File chrome/browser/profiles/profile_info_cache.cc (right):

http://codereview.chromium.org/7155015/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache.cc:22: std::string kNameKey = "name";
No global non-PODs. Just use const char kNameKey[] = ... for these constants.

http://codereview.chromium.org/7155015/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache.cc:37: bool
IsDefaultAvatarIconUrl(const std::string url, size_t* icon_index) {
const std::string&

http://codereview.chromium.org/7155015/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache.cc:57: DCHECK(icon_index <
kDefaultAvatarIconsCount);
DCHECK_LT

http://codereview.chromium.org/7155015/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache.cc:138: DLOG(WARNING) << "Unkown
avatar icon: " << icon_url;
Unknown

http://codereview.chromium.org/7155015/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache.cc:143: void
ProfileInfoCache::SetNameOfProfileAtIndex(size_t index, string16 name) {
const string16&

http://codereview.chromium.org/7155015/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache.cc:163: DCHECK(index <
GetDefaultAvatarIconCount());
DCHECK_LT

http://codereview.chromium.org/7155015/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache.cc:169: DCHECK(index <
GetNumberOfProfiles());
DCHECK_LT

http://codereview.chromium.org/7155015/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache.cc:188: DCHECK(user_data_dir_ ==
profile_path.DirName());
DCHECK_EQ

http://codereview.chromium.org/7155015/diff/1/chrome/browser/profiles/profile...
File chrome/browser/profiles/profile_info_cache.h (right):

http://codereview.chromium.org/7155015/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache.h:27: string16
GetNameOfProfileAtIndex(size_t index) const;
#include "base/string16.h"

http://codereview.chromium.org/7155015/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache.h:42: std::string search_key,
const std::string&

http://codereview.chromium.org/7155015/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache.h:43: string16 search_name);
const std::string&

http://codereview.chromium.org/7155015/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache.h:45: std::vector<std::string>
sorted_keys_;
#include <string>
#include <vector>

http://codereview.chromium.org/7155015/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache.h:48:
DISALLOW_COPY_AND_ASSIGN(ProfileInfoCache);
#include "base/basictypes.h"

http://codereview.chromium.org/7155015/diff/1/chrome/browser/profiles/profile...
File chrome/browser/profiles/profile_info_cache_unittest.cc (right):

http://codereview.chromium.org/7155015/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache_unittest.cc:7: #include
"chrome/browser/profiles/profile_info_cache.h"
Put this include first. See
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Names_...
for explanation (and note the example given of basictypes_test.cc).

http://codereview.chromium.org/7155015/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache_unittest.cc:13: class
ProfileInfoCacheUnittests : public ::testing::Test {
If test code doesn't need to be externally referenced (friended or what not),
then it's probably best to wrap it all in an anonymous namespace to limit its
visibility.

[Miranda Callahan, 2011-06-15 15:16:07]

This is awesome -- I love the addition of a class to show the icons, &c. without
loading the profiles.  A few comments.

http://codereview.chromium.org/7155015/diff/1/chrome/browser/profiles/profile...
File chrome/browser/profiles/profile_info_cache.cc (right):

http://codereview.chromium.org/7155015/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache.cc:36: // returns true and its index
through |icon_index|. If not, returns false.
Ah, I see, you have comments in the ".cc" file -- these are great, could you
just move them to the .h file?

http://codereview.chromium.org/7155015/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache.cc:55: // Returns URL to default
avatar icon with specifided index.
I actually like the word "specifided," but others might complain.

http://codereview.chromium.org/7155015/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache.cc:68: // Popuplate the cache
same w/"Popuplate"

http://codereview.chromium.org/7155015/diff/1/chrome/browser/profiles/profile...
File chrome/browser/profiles/profile_info_cache.h (right):

http://codereview.chromium.org/7155015/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache.h:17: class ProfileInfoCache {
Could you add a class comment here about why this class exists and how it's
used?

http://codereview.chromium.org/7155015/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache.h:35: static int
GetDefaultAvatarIconResourceIDAtIndex(size_t index);
Many of these method names are obvious, but I think some need comments -- for
example, just reading this for the first time, I don't know what "IDA" stands
for. I think I know what the DefaultAvatarIconCount is, but that might also need
a comment.

http://codereview.chromium.org/7155015/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache.h:39: void
SetInfoForProfileAtIndex(size_t index, DictionaryValue* info);
Could you describe what values are supposed to be in |info|? (And are supposed
to be returned from GetInfoForProfileAtIndex?)

http://codereview.chromium.org/7155015/diff/1/chrome/browser/profiles/profile...
File chrome/browser/profiles/profile_info_cache_unittest.cc (right):

http://codereview.chromium.org/7155015/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache_unittest.cc:19: return
StringToFilePath("/usr/profile/directory");
Will this really work on all platforms?

[sail, 2011-06-21 03:26:53]

Hi all, I'm having trouble getting the preference code to work. Currently I get
a crash when reading preferences. Here's the backtrace:
	0   unit_tests                          0x000ef390
base::debug::StackTrace::StackTrace() + 32
	1   unit_tests                          0x02521db4 base::(anonymous
namespace)::StackDumpSignalHandler(int, __siginfo*, __darwin_ucontext*) + 255
	2   libSystem.B.dylib                   0x968831fb _sigtramp + 43
	3   ???                                 0xffffffff 0x0 + 4294967295
	4   unit_tests                          0x000f1ae3
base::internal::LockImpl::Lock() + 27
	5   unit_tests                          0x00035100 base::Lock::Acquire() + 20
	6   unit_tests                          0x00035129
base::AutoLock::AutoLock(base::Lock&) + 27
	7   unit_tests                          0x000f2142
base::ThreadCheckerImpl::EnsureThreadIdAssigned() const + 24
	8   unit_tests                          0x02547ebe
base::ThreadCheckerImpl::CalledOnValidThread() const + 18
	9   unit_tests                          0x02542341
base::NonThreadSafeImpl::CalledOnValidThread() const + 17
	10  unit_tests                          0x013efe2f
PrefService::GetDictionary(char const*) const + 31
	11  unit_tests                          0x01457177
ProfileInfoCache::ProfileInfoCache(FilePath const&) + 127
	12  unit_tests                          0x006c9145 (anonymous
namespace)::ProfileInfoCacheUnittests::ProfileInfoCacheUnittests() + 73

I tried moving the RegisterPrefs() code but it didn't seem to help. Can you see
what I'm doing wrong?

Thanks!

http://codereview.chromium.org/7155015/diff/1/chrome/browser/profiles/profile...
File chrome/browser/profiles/profile_info_cache.cc (right):

http://codereview.chromium.org/7155015/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache.cc:22: std::string kNameKey = "name";
On 2011/06/15 11:22:12, willchan wrote:
> No global non-PODs. Just use const char kNameKey[] = ... for these constants.

Done.

http://codereview.chromium.org/7155015/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache.cc:36: // returns true and its index
through |icon_index|. If not, returns false.
On 2011/06/15 15:16:08, Miranda Callahan wrote:
> Ah, I see, you have comments in the ".cc" file -- these are great, could you
> just move them to the .h file?

Done.

http://codereview.chromium.org/7155015/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache.cc:37: bool
IsDefaultAvatarIconUrl(const std::string url, size_t* icon_index) {
On 2011/06/15 11:22:12, willchan wrote:
> const std::string&

Done.

http://codereview.chromium.org/7155015/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache.cc:55: // Returns URL to default
avatar icon with specifided index.
On 2011/06/15 15:16:08, Miranda Callahan wrote:
> I actually like the word "specifided," but others might complain.

Done.

http://codereview.chromium.org/7155015/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache.cc:57: DCHECK(icon_index <
kDefaultAvatarIconsCount);
On 2011/06/15 11:22:12, willchan wrote:
> DCHECK_LT

Done.

http://codereview.chromium.org/7155015/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache.cc:68: // Popuplate the cache
On 2011/06/15 15:16:08, Miranda Callahan wrote:
> same w/"Popuplate"

Done.

http://codereview.chromium.org/7155015/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache.cc:138: DLOG(WARNING) << "Unkown
avatar icon: " << icon_url;
On 2011/06/15 11:22:12, willchan wrote:
> Unknown

Done.

http://codereview.chromium.org/7155015/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache.cc:143: void
ProfileInfoCache::SetNameOfProfileAtIndex(size_t index, string16 name) {
On 2011/06/15 11:22:12, willchan wrote:
> const string16&

Done.

http://codereview.chromium.org/7155015/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache.cc:163: DCHECK(index <
GetDefaultAvatarIconCount());
On 2011/06/15 11:22:12, willchan wrote:
> DCHECK_LT

Done.

http://codereview.chromium.org/7155015/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache.cc:169: DCHECK(index <
GetNumberOfProfiles());
On 2011/06/15 11:22:12, willchan wrote:
> DCHECK_LT

Done.

http://codereview.chromium.org/7155015/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache.cc:188: DCHECK(user_data_dir_ ==
profile_path.DirName());
On 2011/06/15 11:22:12, willchan wrote:
> DCHECK_EQ

Using DCHECK_EQ didn't work because the macro needs to be able to stream the
value of FilePath. Using DCHECK() seems better in this case.

http://codereview.chromium.org/7155015/diff/1/chrome/browser/profiles/profile...
File chrome/browser/profiles/profile_info_cache.h (right):

http://codereview.chromium.org/7155015/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache.h:17: class ProfileInfoCache {
On 2011/06/15 15:16:08, Miranda Callahan wrote:
> Could you add a class comment here about why this class exists and how it's
> used?

Done.

http://codereview.chromium.org/7155015/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache.h:27: string16
GetNameOfProfileAtIndex(size_t index) const;
On 2011/06/15 11:22:12, willchan wrote:
> #include "base/string16.h"

Done.

http://codereview.chromium.org/7155015/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache.h:35: static int
GetDefaultAvatarIconResourceIDAtIndex(size_t index);
On 2011/06/15 15:16:08, Miranda Callahan wrote:
> Many of these method names are obvious, but I think some need comments -- for
> example, just reading this for the first time, I don't know what "IDA" stands
> for. I think I know what the DefaultAvatarIconCount is, but that might also
need
> a comment.

Done.

http://codereview.chromium.org/7155015/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache.h:39: void
SetInfoForProfileAtIndex(size_t index, DictionaryValue* info);
On 2011/06/15 15:16:08, Miranda Callahan wrote:
> Could you describe what values are supposed to be in |info|? (And are supposed
> to be returned from GetInfoForProfileAtIndex?)

Done.

http://codereview.chromium.org/7155015/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache.h:42: std::string search_key,
On 2011/06/15 11:22:12, willchan wrote:
> const std::string&

Done.

http://codereview.chromium.org/7155015/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache.h:43: string16 search_name);
On 2011/06/15 11:22:12, willchan wrote:
> const std::string&

Done.

http://codereview.chromium.org/7155015/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache.h:45: std::vector<std::string>
sorted_keys_;
On 2011/06/15 11:22:12, willchan wrote:
> #include <string>
> #include <vector>

Done.

http://codereview.chromium.org/7155015/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache.h:48:
DISALLOW_COPY_AND_ASSIGN(ProfileInfoCache);
On 2011/06/15 11:22:12, willchan wrote:
> #include "base/basictypes.h"

Done.

http://codereview.chromium.org/7155015/diff/1/chrome/browser/profiles/profile...
File chrome/browser/profiles/profile_info_cache_unittest.cc (right):

http://codereview.chromium.org/7155015/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache_unittest.cc:7: #include
"chrome/browser/profiles/profile_info_cache.h"
On 2011/06/15 11:22:12, willchan wrote:
> Put this include first. See
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Names_...
> for explanation (and note the example given of basictypes_test.cc).

Done.

http://codereview.chromium.org/7155015/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache_unittest.cc:13: class
ProfileInfoCacheUnittests : public ::testing::Test {
On 2011/06/15 11:22:12, willchan wrote:
> If test code doesn't need to be externally referenced (friended or what not),
> then it's probably best to wrap it all in an anonymous namespace to limit its
> visibility.

Done.

[willchan no longer on Chromium, 2011-06-21 08:26:27]

http://codereview.chromium.org/7155015/diff/4001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_info_cache.cc (right):

http://codereview.chromium.org/7155015/diff/4001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_info_cache.cc:22: const char* kNameKey = "name";
Prefer `static const char kNameKey[] = "name";` over this for two reasons:
* This isn't actually const. It's a non-const pointer to a const char array. If
you use const, then function calls defined by the language standard, such as
strlen(), can actually be completely optimized away by the compiler and replaced
with the actual length.
* Saves a pointer. Typically you don't need a pointer. You just need the const
char array.

http://codereview.chromium.org/7155015/diff/4001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_info_cache.cc:67:
g_browser_process->local_state()->GetDictionary(prefs::kProfileInfoCache);
This is why your test is crashing. g_browser_process is set to TestingProcess
instead of BrowserProcessImpl in unit_tests. local_state() probably returns
NULL. Your best bet is to use dependency injection here. Pass the local state
PrefService in as a dependency to ProfileInfoCache.

http://codereview.chromium.org/7155015/diff/4001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_info_cache_unittest.cc (right):

http://codereview.chromium.org/7155015/diff/4001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_info_cache_unittest.cc:26: #if defined(OS_POSIX)
I think you need build/build_config.h for these preprocessor symbols.

[sail, 2011-06-21 16:28:13]

Awesome! Test now passes on my Mac. Building on Linux and Windows next.

http://codereview.chromium.org/7155015/diff/4001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_info_cache.cc (right):

http://codereview.chromium.org/7155015/diff/4001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_info_cache.cc:22: const char* kNameKey = "name";
On 2011/06/21 08:26:27, willchan wrote:
> Prefer `static const char kNameKey[] = "name";` over this for two reasons:
> * This isn't actually const. It's a non-const pointer to a const char array.
If
> you use const, then function calls defined by the language standard, such as
> strlen(), can actually be completely optimized away by the compiler and
replaced
> with the actual length.
> * Saves a pointer. Typically you don't need a pointer. You just need the const
> char array.

Done.

http://codereview.chromium.org/7155015/diff/4001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_info_cache.cc:67:
g_browser_process->local_state()->GetDictionary(prefs::kProfileInfoCache);
On 2011/06/21 08:26:27, willchan wrote:
> This is why your test is crashing. g_browser_process is set to TestingProcess
> instead of BrowserProcessImpl in unit_tests. local_state() probably returns
> NULL. Your best bet is to use dependency injection here. Pass the local state
> PrefService in as a dependency to ProfileInfoCache.

Done.
Awesome, thanks!

http://codereview.chromium.org/7155015/diff/4001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_info_cache_unittest.cc (right):

http://codereview.chromium.org/7155015/diff/4001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_info_cache_unittest.cc:26: #if defined(OS_POSIX)
On 2011/06/21 08:26:27, willchan wrote:
> I think you need build/build_config.h for these preprocessor symbols.

Testing on windows and linux now.

[sail, 2011-06-22 17:26:21]

Test now passes on windows and linux. Please take another look.

[sail, 2011-06-27 14:12:39]

Ping!

[Miranda Callahan, 2011-06-27 14:52:55]

LGTM -- looks great!

[commit-bot: I haz the power, 2011-06-27 15:59:56]

Try job failure for 7155015-11001 (retry) on linux_clang for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

[commit-bot: I haz the power, 2011-06-27 19:43:42]

Change committed as 90622