Add GAIA info to profile info cache

Add GAIA info to profile info cache

BUG=91241
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=111254

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=111401

[Paweł Hajdan Jr., 2011-11-17 09:56:19]

Drive-by with a testing comment.

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

http://codereview.chromium.org/8587023/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache_unittest.cc:59: class
WaitUntilNotification : public content::NotificationObserver {
Is this just trying to emulate WindowedNotificationObserver? Please just use it.

[Miranda Callahan, 2011-11-17 15:21:10]

Thanks, Sailesh; review comments below. PS -- I rewrote a lot of the shortcut
code yesterday to *not* be contained in ProfileInfoCache; hopefully this will
making merging a little easier.

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

http://codereview.chromium.org/8587023/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache.cc:104: 
These two methods seem really out of place in ProfileInfoCache; is there a
utility class for image manipulation or saving things to disk where it could be
located? If not, maybe it's time to make one -- that way we could directly
combine this code and the code from user_manager.cc, and anybody else
reading/writing images could use it.

http://codereview.chromium.org/8587023/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache.cc:297:
&ProfileInfoCache::OnGAIAPictureLoaded, base::Unretained(this), path);
Could we be heading for trouble by using "Unretained" here (and in other places
in this class)?

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

http://codereview.chromium.org/8587023/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache.h:11: #include <map>
nit: alpha order

http://codereview.chromium.org/8587023/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache.h:78: bool
GetHasMigratedToGAIAInfoOfProfileAtIndex(size_t index) const;
Could you add a comment for these two?

http://codereview.chromium.org/8587023/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache.h:122: void
UpdateSortForProfileIndex(size_t index);
Comment for this one, too.

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

http://codereview.chromium.org/8587023/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_interface.h:41: virtual bool
GetIsUsingGAIANameOfProfileAtIndex(size_t index) const = 0;
Could you add comments, at least for this one and the other "GetIsUsing" method?
Reading these at first glance, I can't immediately tell what it is that they're
supposed to return.

[Robert Sesek, 2011-11-17 20:42:04]

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

http://codereview.chromium.org/8587023/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache.cc:297:
&ProfileInfoCache::OnGAIAPictureLoaded, base::Unretained(this), path);
On 2011/11/17 15:21:10, Miranda Callahan wrote:
> Could we be heading for trouble by using "Unretained" here (and in other
places
> in this class)? 

Definitely. You need to use WeakPtrFactory.

http://codereview.chromium.org/8587023/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache.cc:313:
chrome::NOTIFICATION_PROFILE_CACHED_INFO_CHANGED,
nit: indent 4

http://codereview.chromium.org/8587023/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache.cc:323:
chrome::NOTIFICATION_PROFILE_CACHE_PICTURE_SAVED,
nit: indent 4

http://codereview.chromium.org/8587023/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache.cc:429:
chrome::NOTIFICATION_PROFILE_CACHED_INFO_CHANGED,
nit: indent 4

http://codereview.chromium.org/8587023/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache.cc:633:
chrome::NOTIFICATION_PROFILE_CACHED_INFO_CHANGED,
nit: indent 4

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

http://codereview.chromium.org/8587023/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache.h:21: }
nit: blank line after

http://codereview.chromium.org/8587023/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache.h:57: virtual bool
GetIsUsingGAIANameOfProfileAtIndex(size_t index) const OVERRIDE;
When you return bool, I don't think you need the Get*. Just IsUsing.

http://codereview.chromium.org/8587023/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache.h:60: virtual bool
GetIsUsingGAIAPictureOfProfileAtIndex(
Same here.

http://codereview.chromium.org/8587023/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache.h:78: bool
GetHasMigratedToGAIAInfoOfProfileAtIndex(size_t index) const;
And here.

[sail, 2011-11-21 23:52:47]

addressed review comments, please take another look.

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

http://codereview.chromium.org/8587023/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache.cc:104: 
On 2011/11/17 15:21:10, Miranda Callahan wrote:
> These two methods seem really out of place in ProfileInfoCache; is there a
> utility class for image manipulation or saving things to disk where it could
be
> located? If not, maybe it's time to make one -- that way we could directly
> combine this code and the code from user_manager.cc, and anybody else
> reading/writing images could use it.

I think this is a good idea. Mind if I do this later as a part of the overall
refactoring?

http://codereview.chromium.org/8587023/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache.cc:297:
&ProfileInfoCache::OnGAIAPictureLoaded, base::Unretained(this), path);
On 2011/11/17 20:42:04, rsesek wrote:
> On 2011/11/17 15:21:10, Miranda Callahan wrote:
> > Could we be heading for trouble by using "Unretained" here (and in other
> places
> > in this class)? 
> 
> Definitely. You need to use WeakPtrFactory.

Done.

http://codereview.chromium.org/8587023/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache.cc:313:
chrome::NOTIFICATION_PROFILE_CACHED_INFO_CHANGED,
On 2011/11/17 20:42:04, rsesek wrote:
> nit: indent 4

Done.

http://codereview.chromium.org/8587023/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache.cc:323:
chrome::NOTIFICATION_PROFILE_CACHE_PICTURE_SAVED,
On 2011/11/17 20:42:04, rsesek wrote:
> nit: indent 4

Done.

http://codereview.chromium.org/8587023/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache.cc:429:
chrome::NOTIFICATION_PROFILE_CACHED_INFO_CHANGED,
On 2011/11/17 20:42:04, rsesek wrote:
> nit: indent 4

Done.

http://codereview.chromium.org/8587023/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache.cc:633:
chrome::NOTIFICATION_PROFILE_CACHED_INFO_CHANGED,
On 2011/11/17 20:42:04, rsesek wrote:
> nit: indent 4

Done.

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

http://codereview.chromium.org/8587023/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache.h:11: #include <map>
On 2011/11/17 15:21:10, Miranda Callahan wrote:
> nit: alpha order

Done.

http://codereview.chromium.org/8587023/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache.h:21: }
On 2011/11/17 20:42:04, rsesek wrote:
> nit: blank line after

Done.

http://codereview.chromium.org/8587023/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache.h:57: virtual bool
GetIsUsingGAIANameOfProfileAtIndex(size_t index) const OVERRIDE;
On 2011/11/17 20:42:04, rsesek wrote:
> When you return bool, I don't think you need the Get*. Just IsUsing.

Done.

http://codereview.chromium.org/8587023/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache.h:60: virtual bool
GetIsUsingGAIAPictureOfProfileAtIndex(
On 2011/11/17 20:42:04, rsesek wrote:
> Same here.

Done.

http://codereview.chromium.org/8587023/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache.h:78: bool
GetHasMigratedToGAIAInfoOfProfileAtIndex(size_t index) const;
On 2011/11/17 15:21:10, Miranda Callahan wrote:
> Could you add a comment for these two?

Done.

http://codereview.chromium.org/8587023/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache.h:78: bool
GetHasMigratedToGAIAInfoOfProfileAtIndex(size_t index) const;
On 2011/11/17 20:42:04, rsesek wrote:
> And here.

Done.

http://codereview.chromium.org/8587023/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache.h:122: void
UpdateSortForProfileIndex(size_t index);
On 2011/11/17 15:21:10, Miranda Callahan wrote:
> Comment for this one, too.

Done.

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

http://codereview.chromium.org/8587023/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache_unittest.cc:59: class
WaitUntilNotification : public content::NotificationObserver {
On 2011/11/17 09:56:19, Paweł Hajdan Jr. wrote:
> Is this just trying to emulate WindowedNotificationObserver? Please just use
it.

Done.

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

http://codereview.chromium.org/8587023/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_interface.h:41: virtual bool
GetIsUsingGAIANameOfProfileAtIndex(size_t index) const = 0;
On 2011/11/17 15:21:10, Miranda Callahan wrote:
> Could you add comments, at least for this one and the other "GetIsUsing"
method?
> Reading these at first glance, I can't immediately tell what it is that
they're
> supposed to return.

Done.

[Miranda Callahan, 2011-11-21 23:59:51]

On 2011/11/21 23:52:47, sail wrote:
> addressed review comments, please take another look.
> 
>
http://codereview.chromium.org/8587023/diff/1/chrome/browser/profiles/profile...
> File chrome/browser/profiles/profile_info_cache.cc (right):
> 
>
http://codereview.chromium.org/8587023/diff/1/chrome/browser/profiles/profile...
> chrome/browser/profiles/profile_info_cache.cc:104: 
> On 2011/11/17 15:21:10, Miranda Callahan wrote:
> > These two methods seem really out of place in ProfileInfoCache; is there a
> > utility class for image manipulation or saving things to disk where it could
> be
> > located? If not, maybe it's time to make one -- that way we could directly
> > combine this code and the code from user_manager.cc, and anybody else
> > reading/writing images could use it.
> 
> I think this is a good idea. Mind if I do this later as a part of the overall
> refactoring?
> 
>
http://codereview.chromium.org/8587023/diff/1/chrome/browser/profiles/profile...
> chrome/browser/profiles/profile_info_cache.cc:297:
> &ProfileInfoCache::OnGAIAPictureLoaded, base::Unretained(this), path);
> On 2011/11/17 20:42:04, rsesek wrote:
> > On 2011/11/17 15:21:10, Miranda Callahan wrote:
> > > Could we be heading for trouble by using "Unretained" here (and in other
> > places
> > > in this class)? 
> > 
> > Definitely. You need to use WeakPtrFactory.
> 
> Done.
> 
>
http://codereview.chromium.org/8587023/diff/1/chrome/browser/profiles/profile...
> chrome/browser/profiles/profile_info_cache.cc:313:
> chrome::NOTIFICATION_PROFILE_CACHED_INFO_CHANGED,
> On 2011/11/17 20:42:04, rsesek wrote:
> > nit: indent 4
> 
> Done.
> 
>
http://codereview.chromium.org/8587023/diff/1/chrome/browser/profiles/profile...
> chrome/browser/profiles/profile_info_cache.cc:323:
> chrome::NOTIFICATION_PROFILE_CACHE_PICTURE_SAVED,
> On 2011/11/17 20:42:04, rsesek wrote:
> > nit: indent 4
> 
> Done.
> 
>
http://codereview.chromium.org/8587023/diff/1/chrome/browser/profiles/profile...
> chrome/browser/profiles/profile_info_cache.cc:429:
> chrome::NOTIFICATION_PROFILE_CACHED_INFO_CHANGED,
> On 2011/11/17 20:42:04, rsesek wrote:
> > nit: indent 4
> 
> Done.
> 
>
http://codereview.chromium.org/8587023/diff/1/chrome/browser/profiles/profile...
> chrome/browser/profiles/profile_info_cache.cc:633:
> chrome::NOTIFICATION_PROFILE_CACHED_INFO_CHANGED,
> On 2011/11/17 20:42:04, rsesek wrote:
> > nit: indent 4
> 
> Done.
> 
>
http://codereview.chromium.org/8587023/diff/1/chrome/browser/profiles/profile...
> File chrome/browser/profiles/profile_info_cache.h (right):
> 
>
http://codereview.chromium.org/8587023/diff/1/chrome/browser/profiles/profile...
> chrome/browser/profiles/profile_info_cache.h:11: #include <map>
> On 2011/11/17 15:21:10, Miranda Callahan wrote:
> > nit: alpha order
> 
> Done.
> 
>
http://codereview.chromium.org/8587023/diff/1/chrome/browser/profiles/profile...
> chrome/browser/profiles/profile_info_cache.h:21: }
> On 2011/11/17 20:42:04, rsesek wrote:
> > nit: blank line after
> 
> Done.
> 
>
http://codereview.chromium.org/8587023/diff/1/chrome/browser/profiles/profile...
> chrome/browser/profiles/profile_info_cache.h:57: virtual bool
> GetIsUsingGAIANameOfProfileAtIndex(size_t index) const OVERRIDE;
> On 2011/11/17 20:42:04, rsesek wrote:
> > When you return bool, I don't think you need the Get*. Just IsUsing.
> 
> Done.
> 
>
http://codereview.chromium.org/8587023/diff/1/chrome/browser/profiles/profile...
> chrome/browser/profiles/profile_info_cache.h:60: virtual bool
> GetIsUsingGAIAPictureOfProfileAtIndex(
> On 2011/11/17 20:42:04, rsesek wrote:
> > Same here.
> 
> Done.
> 
>
http://codereview.chromium.org/8587023/diff/1/chrome/browser/profiles/profile...
> chrome/browser/profiles/profile_info_cache.h:78: bool
> GetHasMigratedToGAIAInfoOfProfileAtIndex(size_t index) const;
> On 2011/11/17 15:21:10, Miranda Callahan wrote:
> > Could you add a comment for these two?
> 
> Done.
> 
>
http://codereview.chromium.org/8587023/diff/1/chrome/browser/profiles/profile...
> chrome/browser/profiles/profile_info_cache.h:78: bool
> GetHasMigratedToGAIAInfoOfProfileAtIndex(size_t index) const;
> On 2011/11/17 20:42:04, rsesek wrote:
> > And here.
> 
> Done.
> 
>
http://codereview.chromium.org/8587023/diff/1/chrome/browser/profiles/profile...
> chrome/browser/profiles/profile_info_cache.h:122: void
> UpdateSortForProfileIndex(size_t index);
> On 2011/11/17 15:21:10, Miranda Callahan wrote:
> > Comment for this one, too.
> 
> Done.
> 
>
http://codereview.chromium.org/8587023/diff/1/chrome/browser/profiles/profile...
> File chrome/browser/profiles/profile_info_cache_unittest.cc (right):
> 
>
http://codereview.chromium.org/8587023/diff/1/chrome/browser/profiles/profile...
> chrome/browser/profiles/profile_info_cache_unittest.cc:59: class
> WaitUntilNotification : public content::NotificationObserver {
> On 2011/11/17 09:56:19, Paweł Hajdan Jr. wrote:
> > Is this just trying to emulate WindowedNotificationObserver? Please just use
> it.
> 
> Done.
> 
>
http://codereview.chromium.org/8587023/diff/1/chrome/browser/profiles/profile...
> File chrome/browser/profiles/profile_info_interface.h (right):
> 
>
http://codereview.chromium.org/8587023/diff/1/chrome/browser/profiles/profile...
> chrome/browser/profiles/profile_info_interface.h:41: virtual bool
> GetIsUsingGAIANameOfProfileAtIndex(size_t index) const = 0;
> On 2011/11/17 15:21:10, Miranda Callahan wrote:
> > Could you add comments, at least for this one and the other "GetIsUsing"
> method?
> > Reading these at first glance, I can't immediately tell what it is that
> they're
> > supposed to return.
> 
> Done.

LGTM -- yes, that's fine to do that refactoring in another CL. Thanks!

[Paweł Hajdan Jr., 2011-11-22 09:20:02]

Code I commented in the drive-by LGTM, thanks.

[Robert Sesek, 2011-11-22 13:54:21]

lgtm

[sail, 2011-11-22 19:41:35]

Simplified the code by using PostTaskAndReply() instead of PostTask().

[Robert Sesek, 2011-11-22 21:26:37]

lgtm with nits

http://codereview.chromium.org/8587023/diff/19001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_info_cache.cc (right):

http://codereview.chromium.org/8587023/diff/19001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_info_cache.cc:108: bool *success) {
^T[ *]

http://codereview.chromium.org/8587023/diff/19001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_info_cache.cc:435:
base::Bind(&ProfileInfoCache::OnGAIAPictureSaved, AsWeakPtr(), path,
I'd wrap this by breaking before path and aligning with the &

[sail, 2011-11-22 21:43:23]

http://codereview.chromium.org/8587023/diff/19001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_info_cache.cc (right):

http://codereview.chromium.org/8587023/diff/19001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_info_cache.cc:108: bool *success) {
On 2011/11/22 21:26:37, rsesek wrote:
> ^T[ *]

Done.

http://codereview.chromium.org/8587023/diff/19001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_info_cache.cc:435:
base::Bind(&ProfileInfoCache::OnGAIAPictureSaved, AsWeakPtr(), path,
On 2011/11/22 21:26:37, rsesek wrote:
> I'd wrap this by breaking before path and aligning with the &

Done.

[commit-bot: I haz the power, 2011-11-22 22:04:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/8587023/26002

[commit-bot: I haz the power, 2011-11-22 22:04:39]

Can't process patch for file chrome/test/base/testing_profile_manager.h.
File's status is None, patchset upload is incomplete.

[commit-bot: I haz the power, 2011-11-22 22:11:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/8587023/29009

[commit-bot: I haz the power, 2011-11-22 23:42:00]

Change committed as 111254