Refactor ProfileInfoCache in c/b/profiles

Refactor ProfileInfoCache in most of c/b/profiles

ProfileInfoCache is being refactored into ProfileAttributesStorage and
ProfileAttributesEntry, which use profile paths instead of numerical
indices in the interface. See
https://crrev.com/94dacdb8289038b7fa68c9f2bd57d5311b2cb5cb for details.

TBR=tommi@chromium.org

BUG=305720
Committed: https://crrev.com/143d02516f5a4459076f21bd9557a18638e657de
Cr-Commit-Position: refs/heads/master@{#389279}

Committed: https://crrev.com/498e924981ba4e85ea6c5bc5bcedeea655090743
Cr-Commit-Position: refs/heads/master@{#389376}

[lwchkg, 2016-03-14 14:37:02]

Patchset #1 (id:1) has been deleted

[lwchkg, 2016-03-14 15:34:28]

Patchset #1 (id:20001) has been deleted

[lwchkg, 2016-03-14 15:46:22]

Description was changed from

==========
Refactor ProfileInfoCache in most of c/b/profiles

ProfileInfoCache is being refactored into ProfileAttributesStorage and
ProfileAttributesEntry, which use profile paths instead of numerical
indices in the interface. See
https://crrev.com/94dacdb8289038b7fa68c9f2bd57d5311b2cb5cb for details.

BUG=305720
==========

to

==========
Refactor ProfileInfoCache in most of c/b/profiles

ProfileInfoCache is being refactored into ProfileAttributesStorage and
ProfileAttributesEntry, which use profile paths instead of numerical
indices in the interface. See
https://crrev.com/94dacdb8289038b7fa68c9f2bd57d5311b2cb5cb for details.

BUG=305720
==========

[lwchkg, 2016-03-14 15:46:22]

lwchkg@gmail.com changed reviewers:
+ anthonyvd@chromium.org, mlerman@chromium.org

[lwchkg, 2016-03-14 15:49:10]

Dear all,

  PTAL.

Regards,
WC Leung.

https://codereview.chromium.org/1794353003/diff/40001/chrome/browser/profiles...
File chrome/browser/profiles/gaia_info_update_service.cc (right):

https://codereview.chromium.org/1794353003/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/gaia_info_update_service.cc:124: ProfileAttributesEntry*
entry;
Not sure if any of these returns should be DCHECKs. WDYT?

BTW, it could be legitimate in some of these case for
GetProfileAttributesWithPath to fail, because the profile is deleted by the user
during async operations. In this case we should not change the returns into
DCHECKs.

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

https://codereview.chromium.org/1794353003/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/gaia_info_update_service_unittest.cc:70: class
GAIAInfoUpdateServiceTest : public ProfileInfoCacheTest {
It is a TODO to remove ProfileInfoCacheTest from the test fixture. However, I
need to understand the source and tests in detail before doing this. Should we
add a TODO in the source?

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

https://codereview.chromium.org/1794353003/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager.cc:821: if (profile->GetPath().DirName()
!= user_data_dir())
I want to change this code (and a few similar code) into DCHECK, but I'm not
sure if this is 100% safe.

Anyway, there is no discussion about this line in the related code review:
https://codereview.chromium.org/8771024/

[Mike Lerman, 2016-03-18 17:10:44]

https://codereview.chromium.org/1794353003/diff/40001/chrome/browser/profiles...
File chrome/browser/profiles/gaia_info_update_service.cc (right):

https://codereview.chromium.org/1794353003/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/gaia_info_update_service.cc:124: ProfileAttributesEntry*
entry;
On 2016/03/14 15:49:10, lwchkg wrote:
> Not sure if any of these returns should be DCHECKs. WDYT?
> 
> BTW, it could be legitimate in some of these case for
> GetProfileAttributesWithPath to fail, because the profile is deleted by the
user
> during async operations. In this case we should not change the returns into
> DCHECKs.

Leaving the returns as is makes sense to me.

https://codereview.chromium.org/1794353003/diff/40001/chrome/browser/profiles...
File chrome/browser/profiles/gaia_info_update_service_unittest.cc (left):

https://codereview.chromium.org/1794353003/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/gaia_info_update_service_unittest.cc:231:
TEST_F(GAIAInfoUpdateServiceTest, HandlesProfileReordering) {
I'd almost prefer to leave this in until we've fully removed the old
ProfileInfoCache way of doing things, just to be sure we don't accidentally
regress the way it works.

Leave a TODO, though, to remove it.

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

https://codereview.chromium.org/1794353003/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/gaia_info_update_service_unittest.cc:70: class
GAIAInfoUpdateServiceTest : public ProfileInfoCacheTest {
On 2016/03/14 15:49:10, lwchkg wrote:
> It is a TODO to remove ProfileInfoCacheTest from the test fixture. However, I
> need to understand the source and tests in detail before doing this. Should we
> add a TODO in the source?

sure

https://codereview.chromium.org/1794353003/diff/40001/chrome/browser/profiles...
File chrome/browser/profiles/profile_avatar_downloader.cc (right):

https://codereview.chromium.org/1794353003/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profile_avatar_downloader.cc:56: if
(storage_->GetProfileAttributesWithPath(profile_path_, &entry)) {
just confirming... we don't use this flow for the Guest Profile's avatar?

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

https://codereview.chromium.org/1794353003/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager.cc:694: for (ProfileAttributesEntry*
entry : entries) {
why assign storage.GetAllProfilesAttribures() to a temporary variable, why not
iterate over it directly?
for (ProfileAttributesEntry* entry : storage.GetAllProfilesAttributes())

https://codereview.chromium.org/1794353003/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager.cc:821: if (profile->GetPath().DirName()
!= user_data_dir())
On 2016/03/14 15:49:10, lwchkg wrote:
> I want to change this code (and a few similar code) into DCHECK, but I'm not
> sure if this is 100% safe.
> 
> Anyway, there is no discussion about this line in the related code review:
> https://codereview.chromium.org/8771024/

Maybe this helps with testing?
It seems safe to leave as is.

https://codereview.chromium.org/1794353003/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager.cc:1382: {
you left a set of parens here

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

https://codereview.chromium.org/1794353003/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profile_window.cc:216:
GetAllProfilesAttributesSortedByName();
This is now very fragile, as we're assuming that we're applying the same sort
that's in user_manager_screen_handler's SendUserList function.

[lwchkg, 2016-03-19 18:48:46]

Thanks for review, and sadly I've discovered a bug I introduced into M50.

https://codereview.chromium.org/1794353003/diff/40001/chrome/browser/profiles...
File chrome/browser/profiles/gaia_info_update_service.cc (right):

https://codereview.chromium.org/1794353003/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/gaia_info_update_service.cc:124: ProfileAttributesEntry*
entry;
On 2016/03/18 17:10:44, Mike Lerman wrote:
> On 2016/03/14 15:49:10, lwchkg wrote:
> > Not sure if any of these returns should be DCHECKs. WDYT?
> > 
> > BTW, it could be legitimate in some of these case for
> > GetProfileAttributesWithPath to fail, because the profile is deleted by the
> user
> > during async operations. In this case we should not change the returns into
> > DCHECKs.
> 
> Leaving the returns as is makes sense to me.

Acknowledged.

https://codereview.chromium.org/1794353003/diff/40001/chrome/browser/profiles...
File chrome/browser/profiles/gaia_info_update_service_unittest.cc (left):

https://codereview.chromium.org/1794353003/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/gaia_info_update_service_unittest.cc:231:
TEST_F(GAIAInfoUpdateServiceTest, HandlesProfileReordering) {
On 2016/03/18 17:10:44, Mike Lerman wrote:
> I'd almost prefer to leave this in until we've fully removed the old
> ProfileInfoCache way of doing things, just to be sure we don't accidentally
> regress the way it works.
> 
> Leave a TODO, though, to remove it.

Acknowledged.

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

https://codereview.chromium.org/1794353003/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/gaia_info_update_service_unittest.cc:70: class
GAIAInfoUpdateServiceTest : public ProfileInfoCacheTest {
On 2016/03/18 17:10:44, Mike Lerman wrote:
> On 2016/03/14 15:49:10, lwchkg wrote:
> > It is a TODO to remove ProfileInfoCacheTest from the test fixture. However,
I
> > need to understand the source and tests in detail before doing this. Should
we
> > add a TODO in the source?
> 
> sure

Acknowledged.

https://codereview.chromium.org/1794353003/diff/40001/chrome/browser/profiles...
File chrome/browser/profiles/profile_avatar_downloader.cc (right):

https://codereview.chromium.org/1794353003/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profile_avatar_downloader.cc:56: if
(storage_->GetProfileAttributesWithPath(profile_path_, &entry)) {
On 2016/03/18 17:10:44, Mike Lerman wrote:
> just confirming... we don't use this flow for the Guest Profile's avatar?

Currently no, but it DOES make sense for guests to get an high-res avatar in the
future.

Now only ProfileInfoCache::DownloadHighResAvatar creates an instance of this
class. And according to code search, the call stack is
[1] ProfileInfoCache::DownloadHighResAvatarIfNeeded
[2] ProfileInfoCache::AddProfileToCache or
    ProfileInfoCache::SetAvatarIconOfProfileAtIndex or
    ProfileInfoCache::MigrateLegacyProfileNamesAndDownloadAvatars
and all functions in [2] requires the entry to be present.

Anyway, calling ProfileInfoCache::SaveAvatarImageAtPath by an object owned by
ProfileInfoCache is a bad design. That explicit function call should be made a
callback function to reduce coupling.

Therefore, now I prefer to leave the whole file untouched, and do the
refactoring in a separate CL or when we refactor the profile_info_cache
internal. WDYT?

BTW, do we use these high-res avatar images outside the desktop?

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

https://codereview.chromium.org/1794353003/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager.cc:694: for (ProfileAttributesEntry*
entry : entries) {
On 2016/03/18 17:10:44, Mike Lerman wrote:
> why assign storage.GetAllProfilesAttribures() to a temporary variable, why not
> iterate over it directly?
> for (ProfileAttributesEntry* entry : storage.GetAllProfilesAttributes())

Did think of this, because in other places the code is
g_browser_process->GetProfileManager()->GetProfileAttributesStorage().
    GetAllProfilesAttributes();
which is too long to be included in the for loop.

Sounds good anyway.

https://codereview.chromium.org/1794353003/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager.cc:821: if (profile->GetPath().DirName()
!= user_data_dir())
On 2016/03/18 17:10:44, Mike Lerman wrote:
> On 2016/03/14 15:49:10, lwchkg wrote:
> > I want to change this code (and a few similar code) into DCHECK, but I'm not
> > sure if this is 100% safe.
> > 
> > Anyway, there is no discussion about this line in the related code review:
> > https://codereview.chromium.org/8771024/
> 
> Maybe this helps with testing?

Not sure, but very possible. I guess it *helped* testing. Too bad the author
didn't add a comment for us.

If we want to know, we can try adding LOG(WARNING) and run all tests.

> It seems safe to leave as is.

Agree. And despite my wants to change, leaving the code as is is the only safe
option unless we have more evidence. (And this won't happen soon.)

https://codereview.chromium.org/1794353003/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager.cc:1382: {
On 2016/03/18 17:10:44, Mike Lerman wrote:
> you left a set of parens here

Actually I deliberately added the braces for scoping. I feel uncomfortable to
let entry and has_entry to survive the full function, because another set of
values is assigned to them in line 1409.

WDYT?

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

https://codereview.chromium.org/1794353003/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profile_window.cc:216:
GetAllProfilesAttributesSortedByName();
On 2016/03/18 17:10:44, Mike Lerman wrote:
> This is now very fragile, as we're assuming that we're applying the same sort
> that's in user_manager_screen_handler's SendUserList function.
> 

Agree.

Anyway I've modified that SendUserList to use the same sort function already in
https://crrev.com/dc9dd38953b1cc32bb6d83b6562d8640f3365180, and sadly no one
noticed the code here in that CL.

And therefore M50 has sadly regressed. I'll file an issue tomorrow.

[lwchkg, 2016-03-29 18:43:24]

New patch uploaded. (A few changes are reverted, todos added.)

[Mike Lerman, 2016-03-30 01:18:11]

Man, a lot of changes from rebase crept into patch #2; really hard to tell what
you reverted vs. what's from the patch set.

https://codereview.chromium.org/1794353003/diff/40001/chrome/browser/profiles...
File chrome/browser/profiles/profile_avatar_downloader.cc (right):

https://codereview.chromium.org/1794353003/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profile_avatar_downloader.cc:56: if
(storage_->GetProfileAttributesWithPath(profile_path_, &entry)) {
On 2016/03/19 18:48:46, lwchkg wrote:
> On 2016/03/18 17:10:44, Mike Lerman wrote:
> > just confirming... we don't use this flow for the Guest Profile's avatar?
> 
> Currently no, but it DOES make sense for guests to get an high-res avatar in
the
> future.
> 
> Now only ProfileInfoCache::DownloadHighResAvatar creates an instance of this
> class. And according to code search, the call stack is
> [1] ProfileInfoCache::DownloadHighResAvatarIfNeeded
> [2] ProfileInfoCache::AddProfileToCache or
>     ProfileInfoCache::SetAvatarIconOfProfileAtIndex or
>     ProfileInfoCache::MigrateLegacyProfileNamesAndDownloadAvatars
> and all functions in [2] requires the entry to be present.
> 
> Anyway, calling ProfileInfoCache::SaveAvatarImageAtPath by an object owned by
> ProfileInfoCache is a bad design. That explicit function call should be made a
> callback function to reduce coupling.
> 
> Therefore, now I prefer to leave the whole file untouched, and do the
> refactoring in a separate CL or when we refactor the profile_info_cache
> internal. WDYT?
> 
> BTW, do we use these high-res avatar images outside the desktop?

Sure, leave it as is for now, then. I like reducing the coupling.

Since Guest never appears in UIs along with other profiles as an option to
select, we shouldn't ever need the large-size avatar for it. I'd prefer to still
not download the avatar for Guest/System.

It's Desktop only. I believe there are appropriate #idefs somewhere along the
way to enforce that - look at the top of DownloadHighResAvatarIfNeeded.

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

https://codereview.chromium.org/1794353003/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager.cc:694: for (ProfileAttributesEntry*
entry : entries) {
On 2016/03/19 18:48:46, lwchkg wrote:
> On 2016/03/18 17:10:44, Mike Lerman wrote:
> > why assign storage.GetAllProfilesAttribures() to a temporary variable, why
not
> > iterate over it directly?
> > for (ProfileAttributesEntry* entry : storage.GetAllProfilesAttributes())
> 
> Did think of this, because in other places the code is
> g_browser_process->GetProfileManager()->GetProfileAttributesStorage().
>     GetAllProfilesAttributes();
> which is too long to be included in the for loop.
> 
> Sounds good anyway.

And really, I'm sure the compiler optimizes this anyways.
I'm fine with either one.

https://codereview.chromium.org/1794353003/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager.cc:821: if (profile->GetPath().DirName()
!= user_data_dir())
On 2016/03/19 18:48:46, lwchkg wrote:
> On 2016/03/18 17:10:44, Mike Lerman wrote:
> > On 2016/03/14 15:49:10, lwchkg wrote:
> > > I want to change this code (and a few similar code) into DCHECK, but I'm
not
> > > sure if this is 100% safe.
> > > 
> > > Anyway, there is no discussion about this line in the related code review:
> > > https://codereview.chromium.org/8771024/
> > 
> > Maybe this helps with testing?
> 
> Not sure, but very possible. I guess it *helped* testing. Too bad the author
> didn't add a comment for us.
> 
> If we want to know, we can try adding LOG(WARNING) and run all tests.
> 
> > It seems safe to leave as is.
> 
> Agree. And despite my wants to change, leaving the code as is is the only safe
> option unless we have more evidence. (And this won't happen soon.)

We can add a UserCount and then a Googler can go and check if we ever actually
hit this case.

https://codereview.chromium.org/1794353003/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager.cc:1382: {
On 2016/03/19 18:48:46, lwchkg wrote:
> On 2016/03/18 17:10:44, Mike Lerman wrote:
> > you left a set of parens here
> 
> Actually I deliberately added the braces for scoping. I feel uncomfortable to
> let entry and has_entry to survive the full function, because another set of
> values is assigned to them in line 1409.
> 
> WDYT?

Sure, leave a comment for that please.

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

https://codereview.chromium.org/1794353003/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profile_window.cc:216:
GetAllProfilesAttributesSortedByName();
On 2016/03/19 18:48:46, lwchkg wrote:
> On 2016/03/18 17:10:44, Mike Lerman wrote:
> > This is now very fragile, as we're assuming that we're applying the same
sort
> > that's in user_manager_screen_handler's SendUserList function.
> > 
> 
> Agree.
> 
> Anyway I've modified that SendUserList to use the same sort function already
in
> https://crrev.com/dc9dd38953b1cc32bb6d83b6562d8640f3365180, and sadly no one
> noticed the code here in that CL.
> 
> And therefore M50 has sadly regressed. I'll file an issue tomorrow.

Let's wait for your other related CL to land first, then rebase this CL onto
that, and then review how this CL looks.

https://codereview.chromium.org/1794353003/diff/60001/chrome/browser/media/ta...
File chrome/browser/media/tab_desktop_media_list.cc (right):

https://codereview.chromium.org/1794353003/diff/60001/chrome/browser/media/ta...
chrome/browser/media/tab_desktop_media_list.cc:29: gfx::ImageSkia
CreateEnclosedFaviconImage(gfx::Size size,
Did these changes come from a rebase?

THis is why you always do rebases in their own patch ;)

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

https://codereview.chromium.org/1794353003/diff/60001/chrome/browser/profiles...
chrome/browser/profiles/gaia_info_update_service_unittest.cc:109: static
std::string GivenName(const std::string& id) {
These functions don't at all explain what they're doing... poor names. I'd just
use the appropriate literal strings.

https://codereview.chromium.org/1794353003/diff/60001/chrome/browser/profiles...
chrome/browser/profiles/gaia_info_update_service_unittest.cc:253: size_t
index_before =
before what? the index of the profile before mine?
Maybe original_profile_index

[lwchkg, 2016-03-31 19:32:26]

Sorry for the poor rebase. Will rebase again tomorrow after all the related CL
lands.

Anyway, I am likely unable to get a pure rebase of patch #1 because of merge
conflicts.

https://codereview.chromium.org/1794353003/diff/40001/chrome/browser/profiles...
File chrome/browser/profiles/profile_avatar_downloader.cc (right):

https://codereview.chromium.org/1794353003/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profile_avatar_downloader.cc:56: if
(storage_->GetProfileAttributesWithPath(profile_path_, &entry)) {
On 2016/03/30 01:18:10, Mike Lerman wrote:
> On 2016/03/19 18:48:46, lwchkg wrote:
> > On 2016/03/18 17:10:44, Mike Lerman wrote:
> > > just confirming... we don't use this flow for the Guest Profile's avatar?
> > 
> > Currently no, but it DOES make sense for guests to get an high-res avatar in
> the
> > future.
> > 
> > Now only ProfileInfoCache::DownloadHighResAvatar creates an instance of this
> > class. And according to code search, the call stack is
> > [1] ProfileInfoCache::DownloadHighResAvatarIfNeeded
> > [2] ProfileInfoCache::AddProfileToCache or
> >     ProfileInfoCache::SetAvatarIconOfProfileAtIndex or
> >     ProfileInfoCache::MigrateLegacyProfileNamesAndDownloadAvatars
> > and all functions in [2] requires the entry to be present.
> > 
> > Anyway, calling ProfileInfoCache::SaveAvatarImageAtPath by an object owned
by
> > ProfileInfoCache is a bad design. That explicit function call should be made
a
> > callback function to reduce coupling.
> > 
> > Therefore, now I prefer to leave the whole file untouched, and do the
> > refactoring in a separate CL or when we refactor the profile_info_cache
> > internal. WDYT?
> > 
> > BTW, do we use these high-res avatar images outside the desktop?
> 
> Sure, leave it as is for now, then. I like reducing the coupling.
> 
> Since Guest never appears in UIs along with other profiles as an option to
> select, we shouldn't ever need the large-size avatar for it. I'd prefer to
still
> not download the avatar for Guest/System.

We have those funny anonymous animals in Google Docs/Sheet. Not sure if the UI
guys will ask for a similar humor in the future. :-)

Anyway, profile_path is already refactored out of ProfileAvatarDownloader, so
the discussion is now moot.

> 
> It's Desktop only. I believe there are appropriate #idefs somewhere along the
> way to enforce that - look at the top of DownloadHighResAvatarIfNeeded.

Thanks.

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

https://codereview.chromium.org/1794353003/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager.cc:694: for (ProfileAttributesEntry*
entry : entries) {
> > Did think of this, because in other places the code is

Stupid typo. Should be "Didn't think of this" (i.e. the opposite) instead.

https://codereview.chromium.org/1794353003/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager.cc:821: if (profile->GetPath().DirName()
!= user_data_dir())
Sorry... what is a UserCount?

> We can add a UserCount and then a Googler can go and check if we ever actually
> hit this case.

https://codereview.chromium.org/1794353003/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager.cc:1382: {
On 2016/03/30 01:18:10, Mike Lerman wrote:
> On 2016/03/19 18:48:46, lwchkg wrote:
> > On 2016/03/18 17:10:44, Mike Lerman wrote:
> > > you left a set of parens here
> > 
> > Actually I deliberately added the braces for scoping. I feel uncomfortable
to
> > let entry and has_entry to survive the full function, because another set of
> > values is assigned to them in line 1409.
> > 
> > WDYT?
> 
> Sure, leave a comment for that please.

Acknowledged.

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

https://codereview.chromium.org/1794353003/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profile_window.cc:216:
GetAllProfilesAttributesSortedByName();
On 2016/03/30 01:18:10, Mike Lerman wrote:
> On 2016/03/19 18:48:46, lwchkg wrote:
> > On 2016/03/18 17:10:44, Mike Lerman wrote:
> > > This is now very fragile, as we're assuming that we're applying the same
> sort
> > > that's in user_manager_screen_handler's SendUserList function.
> > > 
> > 
> > Agree.
> > 
> > Anyway I've modified that SendUserList to use the same sort function already
> in
> > https://crrev.com/dc9dd38953b1cc32bb6d83b6562d8640f3365180, and sadly no one
> > noticed the code here in that CL.
> > 
> > And therefore M50 has sadly regressed. I'll file an issue tomorrow.
> 
> Let's wait for your other related CL to land first, then rebase this CL onto
> that, and then review how this CL looks.

Sure. Will rebase later today.

https://codereview.chromium.org/1794353003/diff/60001/chrome/browser/media/ta...
File chrome/browser/media/tab_desktop_media_list.cc (right):

https://codereview.chromium.org/1794353003/diff/60001/chrome/browser/media/ta...
chrome/browser/media/tab_desktop_media_list.cc:29: gfx::ImageSkia
CreateEnclosedFaviconImage(gfx::Size size,
On 2016/03/30 01:18:10, Mike Lerman wrote:
> Did these changes come from a rebase?
> 
> THis is why you always do rebases in their own patch ;)

Yes. The only thing I've done to this file is to add a #include.

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

https://codereview.chromium.org/1794353003/diff/60001/chrome/browser/profiles...
chrome/browser/profiles/gaia_info_update_service_unittest.cc:109: static
std::string GivenName(const std::string& id) {
On 2016/03/30 01:18:11, Mike Lerman wrote:
> These functions don't at all explain what they're doing... poor names. I'd
just
> use the appropriate literal strings.

Sorry for poor rebase. These functions are used by a test which is marked with a
todo of remove. So I'd rather leave them here temporarily. (Likely to be removed
in 1-2 months if I continue to refactor ProfileInfoCache internals.)

Anyway, I don't understand why are the concept of "first name" and "last name"
is here. We simply don't have such a concept in profiles.

https://codereview.chromium.org/1794353003/diff/60001/chrome/browser/profiles...
chrome/browser/profiles/gaia_info_update_service_unittest.cc:253: size_t
index_before =
On 2016/03/30 01:18:11, Mike Lerman wrote:
> before what? the index of the profile before mine?
> Maybe original_profile_index

Sorry again for poor rebase. The whole test looks strange anyway.

[Mike Lerman, 2016-04-04 15:45:31]

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

https://codereview.chromium.org/1794353003/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager.cc:821: if (profile->GetPath().DirName()
!= user_data_dir())
On 2016/03/31 19:32:26, lwchkg wrote:
> Sorry... what is a UserCount?
> 
> > We can add a UserCount and then a Googler can go and check if we ever
actually
> > hit this case.
> 

It's a way to gather statistics from the Chrome userbase (for those who opt in).
The way to do so is with a call to UMA_HISTOGRAM_BOOLEAN(). You can set up a new
histogram in histograms.xml. You can verify locally by looking at
chrome://histograms and seeing what Chrome's taking note of (note: it always
tracks locally whether you've opted in or not, and uploads to Google only if
you've opted in)

[lwchkg, 2016-04-06 16:17:52]

Patches 3-5 uploaded. PTAL.

Anyway, I did not add the UMA_HISTOGRAM yet. I'll locally add some LOG(WARNING)
and identify the tests that execute those code paths. If it does not reveal any
potential non-test code using the code paths, then I'll add the histograms.

BTW, I've also found a test likely using the quirky behavior:
ProfileChooserViewExtensionsTest.NoProfileChooserOnOutsideUserDataDirProfiles
Sadly I don't have the rights to read crbug.com/527505, but the CL suggest the
code is added with a heap-use-after-free bug fix.

[lwchkg, 2016-04-07 16:10:37]

Uploaded patch #6 because the browser tests lasts too long for my computer
(looks like a half day or more.) So can you please run a CQ dry run of this
patch? Thanks.

[Mike Lerman, 2016-04-07 17:04:26]

The CQ bit was checked by mlerman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-07 17:04:54]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1794353003/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1794353003/140001

[commit-bot: I haz the power, 2016-04-07 17:16:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-07 17:16:07]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[lwchkg, 2016-04-07 18:15:52]

lwchkg@gmail.com changed reviewers:
+ rogerta@chromium.org

[lwchkg, 2016-04-07 18:15:53]

+rogerta

Dear Roger,

  PTAL on signin_create_profile_handler.cc. It's a follow-up of
https://crrev.com/1d0bb14e8121201dd393eef0f43834327702815c

  Anyway Mike/Roger: please start a CQ dry run for me. I need to know where does
the LOG(WARNING) appear in the test log.

Regards,
WC Leung.

c.c. Moe.

[lwchkg, 2016-04-12 13:14:20]

On 2016/04/06 16:17:52, lwchkg wrote:
> Patches 3-5 uploaded. PTAL.
> 
> Anyway, I did not add the UMA_HISTOGRAM yet. I'll locally add some
LOG(WARNING)
> and identify the tests that execute those code paths. If it does not reveal
any
> potential non-test code using the code paths, then I'll add the histograms.
> 

Just conducted the tests in my computer. Nothing is revealed when I run the
whole pack of tests. However, some LOG(WARNING) are dumped if I execute
ProfileChooserViewExtensionsTest.NoProfileChooserOnOutsideUserDataDirProfiles
individually. Strange.

Anyway, it means no non-test code needs our attention. So I'll add the
histograms now. Meanwhile, can you help to make a CQ dry run of patch #8? (Our
last attempt ended up in a conflict with Moe's CL.)

[lwchkg, 2016-04-12 14:09:03]

Dear Mike,

  New patch uploaded. PTAL. (Note: you might want to comment about the naming
and descriptions of the histograms.)

Regards,
WC Leung.

[Mike Lerman, 2016-04-13 15:07:42]

https://codereview.chromium.org/1794353003/diff/200001/chrome/browser/profile...
File chrome/browser/profiles/gaia_info_update_service.cc (right):

https://codereview.chromium.org/1794353003/diff/200001/chrome/browser/profile...
chrome/browser/profiles/gaia_info_update_service.cc:206: // These metrics should
logged only on this schedule.
Put this back... we do really want to log them at any time, even if it's more
often than the schedule.

Also, we may not always do a ProfileDownload (e.g. the user may not be signed
in, and thus not have a profile image to download from GAIA).

https://codereview.chromium.org/1794353003/diff/200001/chrome/browser/profile...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/1794353003/diff/200001/chrome/browser/profile...
chrome/browser/profiles/profile_manager.cc:877: // TODO(anthonyvd) : monitor the
histogram below to see whether the code path
Don't need this TODO.

https://codereview.chromium.org/1794353003/diff/200001/chrome/browser/profile...
chrome/browser/profiles/profile_manager.cc:883:
UMA_HISTOGRAM_BOOLEAN("Profile.InitProfileUserPrefs.OutsideUserDir", false);
don't log the false.

https://codereview.chromium.org/1794353003/diff/200001/chrome/browser/profile...
chrome/browser/profiles/profile_manager.cc:1434:
UMA_HISTOGRAM_BOOLEAN("Profile.GetProfileInfoPath.OutsideUserDir", true);
Same as above.

https://codereview.chromium.org/1794353003/diff/200001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1794353003/diff/200001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:40565: +    user data directory. We want
to know if the true case ever happens.
We expect this to never happen but need to verify in stable. (and below)

[lwchkg, 2016-04-13 16:50:57]

Thanks for review. A new patch is uploaded. PTAL.

https://codereview.chromium.org/1794353003/diff/200001/chrome/browser/profile...
File chrome/browser/profiles/gaia_info_update_service.cc (right):

https://codereview.chromium.org/1794353003/diff/200001/chrome/browser/profile...
chrome/browser/profiles/gaia_info_update_service.cc:206: // These metrics should
logged only on this schedule.
On 2016/04/13 15:07:41, Mike Lerman wrote:
> Put this back... we do really want to log them at any time, even if it's more
> often than the schedule.
> 
> Also, we may not always do a ProfileDownload (e.g. the user may not be signed
> in, and thus not have a profile image to download from GAIA).

See
https://chromium.googlesource.com/chromium/src/+log/master/chrome/browser/pro...

The code here was added by rogerta in fab2d8a, and then reverted by nednguyen in
b7f41da for failing mac perf benchmarks (the reversion is not shown in this CL).

Unless there's a merge conflict, I think we can safely ignore this change.

https://codereview.chromium.org/1794353003/diff/200001/chrome/browser/profile...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/1794353003/diff/200001/chrome/browser/profile...
chrome/browser/profiles/profile_manager.cc:877: // TODO(anthonyvd) : monitor the
histogram below to see whether the code path
On 2016/04/13 15:07:41, Mike Lerman wrote:
> Don't need this TODO.

Done.

https://codereview.chromium.org/1794353003/diff/200001/chrome/browser/profile...
chrome/browser/profiles/profile_manager.cc:883:
UMA_HISTOGRAM_BOOLEAN("Profile.InitProfileUserPrefs.OutsideUserDir", false);
On 2016/04/13 15:07:41, Mike Lerman wrote:
> don't log the false.

Done.

https://codereview.chromium.org/1794353003/diff/200001/chrome/browser/profile...
chrome/browser/profiles/profile_manager.cc:1434:
UMA_HISTOGRAM_BOOLEAN("Profile.GetProfileInfoPath.OutsideUserDir", true);
On 2016/04/13 15:07:41, Mike Lerman wrote:
> Same as above.

Done.

https://codereview.chromium.org/1794353003/diff/200001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1794353003/diff/200001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:40565: +    user data directory. We want
to know if the true case ever happens.
On 2016/04/13 15:07:41, Mike Lerman wrote:
> We expect this to never happen but need to verify in stable. (and below)

Done.

[Mike Lerman, 2016-04-18 14:34:40]

Okay. I somewhat nervously say LGTM :)

[lwchkg, 2016-04-18 15:20:41]

On 2016/04/18 14:34:40, Mike Lerman wrote:
> Okay. I somewhat nervously say LGTM :)

Nervously? Hears like something smells.
Anyway I'll start the trybots. When it's green I'll invite the other owners to
review.

[lwchkg, 2016-04-18 15:21:12]

The CQ bit was checked by lwchkg@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2016-04-18 15:21:37]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1794353003/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1794353003/220001

[Mike Lerman, 2016-04-18 15:23:52]

On 2016/04/18 15:21:37, commit-bot: I haz the power wrote:
> Dry run: CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/1794353003/220001
> View timeline at
>  https://chromium-cq-status.appspot.com/patch-timeline/1794353003/220001

Not really, it's just a big CL and a lot's changed with rebases (because I took
a long time to review, I know)

[commit-bot: I haz the power, 2016-04-18 15:41:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-18 15:41:27]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[lwchkg, 2016-04-18 15:45:14]

On 2016/04/18 15:23:52, Mike Lerman wrote:
> On 2016/04/18 15:21:37, commit-bot: I haz the power wrote:
> > Dry run: CQ is trying da patch. Follow status at
> >  https://chromium-cq-status.appspot.com/patch-status/1794353003/220001
> > View timeline at
> >  https://chromium-cq-status.appspot.com/patch-timeline/1794353003/220001
> 
> Not really, it's just a big CL and a lot's changed with rebases (because I
took
> a long time to review, I know)

OIC. Let's hope that there won't be a giant bug report filed on this CL. Once
the CL has landed, we can hide ProfileInfoCache from the outside (plus
refactoring a few missed files).

Anyway, rebased a lot because of conflicts. :-(

[lwchkg, 2016-04-18 15:56:50]

The CQ bit was checked by lwchkg@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2016-04-18 15:57:23]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1794353003/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1794353003/240001

[commit-bot: I haz the power, 2016-04-18 16:08:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-18 16:08:49]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[lwchkg, 2016-04-18 23:15:33]

The CQ bit was checked by lwchkg@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2016-04-18 23:15:50]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1794353003/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1794353003/260001

[commit-bot: I haz the power, 2016-04-19 00:38:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-19 00:38:31]

Dry run: This issue passed the CQ dry run.

[lwchkg, 2016-04-20 13:25:36]

lwchkg@gmail.com changed reviewers:
+ dewittj@chromium.org, isherman@chromium.org, rsesek@chromium.org,
tommi@chromium.org

[lwchkg, 2016-04-20 14:03:16]

+owners of files

Dear all,

  PTAL on this CL. Thanks. Here are the files you own:

Robert Sesek: chrome/browser/app_controller_mac_browsertest.mm
dewittj@: chrome/browser/notifications/platform_notification_service_impl.cc
tommi@: chrome/browser/media/tab_desktop_media_list.cc
(Note: the above three files are all about adding missing #include(s).)

Ilya Sherman: histograms, and their use in
chrome/browser/profiles/profile_manager.cc (lines 878 and 1429)

Regards,
WC Leung.

Roger: last time I've asked you to signin_create_profile_handler.cc, but the
file is also owned by Mike. So you may feel free to skip my message. Sorry for
inconvenience.

[Mike Lerman, 2016-04-20 14:08:25]

On 2016/04/20 14:03:16, lwchkg wrote:
> +owners of files
> 
> Dear all,
> 
>   PTAL on this CL. Thanks. Here are the files you own:
> 
> Robert Sesek: chrome/browser/app_controller_mac_browsertest.mm
> dewittj@: chrome/browser/notifications/platform_notification_service_impl.cc
> tommi@: chrome/browser/media/tab_desktop_media_list.cc
> (Note: the above three files are all about adding missing #include(s).)
> 
> Ilya Sherman: histograms, and their use in
> chrome/browser/profiles/profile_manager.cc (lines 878 and 1429)
> 
> Regards,
> WC Leung.
> 
> Roger: last time I've asked you to signin_create_profile_handler.cc, but the
> file is also owned by Mike. So you may feel free to skip my message. Sorry for
> inconvenience.

Feel free to TBR people for those missing includes.

But do make sure to get review for histograms.xml.

[dewittj, 2016-04-20 14:30:08]

notifications lgtm

[Robert Sesek, 2016-04-20 14:43:53]

app_controller_mac_browsertest.mm LGTM

[Ilya Sherman, 2016-04-20 23:21:40]

histograms LGTM % nits:

https://codereview.chromium.org/1794353003/diff/260001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1794353003/diff/260001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:40561: +    enum="BooleanEnabled">
nit: I think BooleanHit would be a better choice of enum for this histogram.

https://codereview.chromium.org/1794353003/diff/260001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:40564: +    Whether
Profile::InitProfileUserPrefs is called with a profile outside the
nit: Should this be Profile::GetProfileInfoPath?

https://codereview.chromium.org/1794353003/diff/260001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:40566: +    stable.
It sounds like this is meant to be a temporary histogram.  If so, please add a
note indicating what milestone it's expected to be safe to remove in.  That way,
even if you get distracted from getting back to this and cleaning it up, someone
else can pretty safely judge that the cleanup is safe, and do so.

https://codereview.chromium.org/1794353003/diff/260001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:40577: +    enum="BooleanEnabled">
(Ditto)

https://codereview.chromium.org/1794353003/diff/260001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:40582: +    stable.
(Ditto)

[Mike Lerman, 2016-04-20 23:47:14]

https://codereview.chromium.org/1794353003/diff/260001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1794353003/diff/260001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:40566: +    stable.
On 2016/04/20 23:21:40, Ilya Sherman wrote:
> It sounds like this is meant to be a temporary histogram.  If so, please add a
> note indicating what milestone it's expected to be safe to remove in.  That
way,
> even if you get distracted from getting back to this and cleaning it up,
someone
> else can pretty safely judge that the cleanup is safe, and do so.

Let's say M53.

[lwchkg, 2016-04-21 17:22:07]

New patch uploaded.

Mike: can you help to confirm the meaning of "M53" in the comment below? Thanks.

https://codereview.chromium.org/1794353003/diff/260001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1794353003/diff/260001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:40561: +    enum="BooleanEnabled">
On 2016/04/20 23:21:40, Ilya Sherman wrote:
> nit: I think BooleanHit would be a better choice of enum for this histogram.

Done.

https://codereview.chromium.org/1794353003/diff/260001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:40564: +    Whether
Profile::InitProfileUserPrefs is called with a profile outside the
On 2016/04/20 23:21:40, Ilya Sherman wrote:
> nit: Should this be Profile::GetProfileInfoPath?

Yes, thanks for spotting. Done.

https://codereview.chromium.org/1794353003/diff/260001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:40566: +    stable.
On 2016/04/20 23:47:14, Mike Lerman wrote:
> On 2016/04/20 23:21:40, Ilya Sherman wrote:
> > It sounds like this is meant to be a temporary histogram.  If so, please add
a
> > note indicating what milestone it's expected to be safe to remove in.  That
> way,
> > even if you get distracted from getting back to this and cleaning it up,
> someone
> > else can pretty safely judge that the cleanup is safe, and do so.
> 
> Let's say M53.

Done. BTW, Mike: do you mean that the code will stay in stable for the duration
of one release, i.e. M52?

https://codereview.chromium.org/1794353003/diff/260001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:40577: +    enum="BooleanEnabled">
On 2016/04/20 23:21:40, Ilya Sherman wrote:
> (Ditto)

Done.

https://codereview.chromium.org/1794353003/diff/260001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:40582: +    stable.
On 2016/04/20 23:21:40, Ilya Sherman wrote:
> (Ditto)

Done.

[Mike Lerman, 2016-04-21 17:32:51]

https://codereview.chromium.org/1794353003/diff/260001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1794353003/diff/260001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:40566: +    stable.
On 2016/04/21 17:22:07, lwchkg wrote:
> On 2016/04/20 23:47:14, Mike Lerman wrote:
> > On 2016/04/20 23:21:40, Ilya Sherman wrote:
> > > It sounds like this is meant to be a temporary histogram.  If so, please
add
> a
> > > note indicating what milestone it's expected to be safe to remove in. 
That
> > way,
> > > even if you get distracted from getting back to this and cleaning it up,
> > someone
> > > else can pretty safely judge that the cleanup is safe, and do so.
> > 
> > Let's say M53.
> 
> Done. BTW, Mike: do you mean that the code will stay in stable for the
duration
> of one release, i.e. M52?

Let's say M54 to be safe. This means you can leave a comment in that says "This
metric may be removed after M54". We then need to remember to take a look at the
resulting values over the next couple of months :)

[lwchkg, 2016-04-21 17:59:28]

https://codereview.chromium.org/1794353003/diff/260001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1794353003/diff/260001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:40566: +    stable.
On 2016/04/21 17:32:51, Mike Lerman wrote:
> On 2016/04/21 17:22:07, lwchkg wrote:
> > Done. BTW, Mike: do you mean that the code will stay in stable for the
> duration
> > of one release, i.e. M52?
> 
> Let's say M54 to be safe. This means you can leave a comment in that says
"This
> metric may be removed after M54". We then need to remember to take a look at
the
> resulting values over the next couple of months :)

Done. Anyways, I envy that your English is better than mine. :-)

[lwchkg, 2016-04-21 17:59:36]

The CQ bit was checked by lwchkg@gmail.com

[lwchkg, 2016-04-21 17:59:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlerman@chromium.org, rsesek@chromium.org, isherman@chromium.org,
dewittj@chromium.org
Link to the patchset: https://codereview.chromium.org/1794353003/#ps300001
(title: "Updated histograms (again) according to comment.")

[commit-bot: I haz the power, 2016-04-21 18:00:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1794353003/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1794353003/300001

[commit-bot: I haz the power, 2016-04-21 18:09:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-21 18:09:46]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[lwchkg, 2016-04-22 13:36:31]

The CQ bit was checked by lwchkg@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2016-04-22 13:36:42]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1794353003/320001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1794353003/320001

[lwchkg, 2016-04-22 13:36:53]

Description was changed from

==========
Refactor ProfileInfoCache in most of c/b/profiles

ProfileInfoCache is being refactored into ProfileAttributesStorage and
ProfileAttributesEntry, which use profile paths instead of numerical
indices in the interface. See
https://crrev.com/94dacdb8289038b7fa68c9f2bd57d5311b2cb5cb for details.

BUG=305720
==========

to

==========
Refactor ProfileInfoCache in most of c/b/profiles

ProfileInfoCache is being refactored into ProfileAttributesStorage and
ProfileAttributesEntry, which use profile paths instead of numerical
indices in the interface. See
https://crrev.com/94dacdb8289038b7fa68c9f2bd57d5311b2cb5cb for details.

TBR=tommi@chromium.org

BUG=305720
==========

[lwchkg, 2016-04-22 14:16:16]

New patch uploaded with one file added. Mike: PTAL at
https://codereview.chromium.org/1794353003/diff/320001/chrome/browser/ui/webu...
. Thanks!

[commit-bot: I haz the power, 2016-04-22 15:04:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-22 15:04:15]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[lwchkg, 2016-04-22 15:27:41]

The CQ bit was checked by lwchkg@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2016-04-22 15:28:10]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1794353003/320001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1794353003/320001

[commit-bot: I haz the power, 2016-04-22 16:08:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-22 16:08:40]

Dry run: This issue passed the CQ dry run.

[lwchkg, 2016-04-22 22:40:35]

The CQ bit was checked by lwchkg@gmail.com

[lwchkg, 2016-04-22 22:40:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlerman@chromium.org, rsesek@chromium.org, isherman@chromium.org,
dewittj@chromium.org
Link to the patchset: https://codereview.chromium.org/1794353003/#ps320001
(title: "Rebased and added signin_create_profile_handler_unittest.cc because of
compile failure")

[commit-bot: I haz the power, 2016-04-22 22:41:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1794353003/320001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1794353003/320001

[commit-bot: I haz the power, 2016-04-22 22:48:08]

Description was changed from

==========
Refactor ProfileInfoCache in most of c/b/profiles

ProfileInfoCache is being refactored into ProfileAttributesStorage and
ProfileAttributesEntry, which use profile paths instead of numerical
indices in the interface. See
https://crrev.com/94dacdb8289038b7fa68c9f2bd57d5311b2cb5cb for details.

TBR=tommi@chromium.org

BUG=305720
==========

to

==========
Refactor ProfileInfoCache in most of c/b/profiles

ProfileInfoCache is being refactored into ProfileAttributesStorage and
ProfileAttributesEntry, which use profile paths instead of numerical
indices in the interface. See
https://crrev.com/94dacdb8289038b7fa68c9f2bd57d5311b2cb5cb for details.

TBR=tommi@chromium.org

BUG=305720
==========

[commit-bot: I haz the power, 2016-04-22 22:48:11]

Committed patchset #15 (id:320001)

[commit-bot: I haz the power, 2016-04-22 22:50:05]

Description was changed from

==========
Refactor ProfileInfoCache in most of c/b/profiles

ProfileInfoCache is being refactored into ProfileAttributesStorage and
ProfileAttributesEntry, which use profile paths instead of numerical
indices in the interface. See
https://crrev.com/94dacdb8289038b7fa68c9f2bd57d5311b2cb5cb for details.

TBR=tommi@chromium.org

BUG=305720
==========

to

==========
Refactor ProfileInfoCache in most of c/b/profiles

ProfileInfoCache is being refactored into ProfileAttributesStorage and
ProfileAttributesEntry, which use profile paths instead of numerical
indices in the interface. See
https://crrev.com/94dacdb8289038b7fa68c9f2bd57d5311b2cb5cb for details.

TBR=tommi@chromium.org

BUG=305720

Committed: https://crrev.com/143d02516f5a4459076f21bd9557a18638e657de
Cr-Commit-Position: refs/heads/master@{#389279}
==========

[commit-bot: I haz the power, 2016-04-22 22:50:07]

Patchset 15 (id:??) landed as
https://crrev.com/143d02516f5a4459076f21bd9557a18638e657de
Cr-Commit-Position: refs/heads/master@{#389279}

[apacible, 2016-04-22 23:39:26]

Some GAIAInfoUpdateServiceTests are failing on Mac 10.9. I'm manually reverting
since the patch is large.

https://build.chromium.org/p/chromium.mac/builders/Mac10.9%20Tests/builds/20191
https://build.chromium.org/p/chromium.memory/builders/Mac%20ASan%2064%20Tests...

[apacible, 2016-04-22 23:47:16]

On 2016/04/22 23:39:26, apacible wrote:
> Some GAIAInfoUpdateServiceTests are failing on Mac 10.9. I'm manually
reverting
> since the patch is large.
> 
>
https://build.chromium.org/p/chromium.mac/builders/Mac10.9%20Tests/builds/20191
>
https://build.chromium.org/p/chromium.memory/builders/Mac%20ASan%2064%20Tests...

Reverted in https://codereview.chromium.org/1913373002/.

[lwchkg, 2016-04-22 23:47:57]

Description was changed from

==========
Refactor ProfileInfoCache in most of c/b/profiles

ProfileInfoCache is being refactored into ProfileAttributesStorage and
ProfileAttributesEntry, which use profile paths instead of numerical
indices in the interface. See
https://crrev.com/94dacdb8289038b7fa68c9f2bd57d5311b2cb5cb for details.

TBR=tommi@chromium.org

BUG=305720

Committed: https://crrev.com/143d02516f5a4459076f21bd9557a18638e657de
Cr-Commit-Position: refs/heads/master@{#389279}
==========

to

==========
Refactor ProfileInfoCache in most of c/b/profiles

ProfileInfoCache is being refactored into ProfileAttributesStorage and
ProfileAttributesEntry, which use profile paths instead of numerical
indices in the interface. See
https://crrev.com/94dacdb8289038b7fa68c9f2bd57d5311b2cb5cb for details.

TBR=tommi@chromium.org

BUG=305720

Committed: https://crrev.com/143d02516f5a4459076f21bd9557a18638e657de
Cr-Commit-Position: refs/heads/master@{#389279}
==========

[lwchkg, 2016-04-23 09:47:07]

The CQ bit was checked by lwchkg@gmail.com

[lwchkg, 2016-04-23 09:47:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlerman@chromium.org, rsesek@chromium.org, isherman@chromium.org,
dewittj@chromium.org
Link to the patchset: https://codereview.chromium.org/1794353003/#ps340001
(title: "Fix a bug that causes unit tests without debug code to fail.")

[commit-bot: I haz the power, 2016-04-23 09:47:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1794353003/340001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1794353003/340001

[commit-bot: I haz the power, 2016-04-23 11:04:27]

Description was changed from

==========
Refactor ProfileInfoCache in most of c/b/profiles

ProfileInfoCache is being refactored into ProfileAttributesStorage and
ProfileAttributesEntry, which use profile paths instead of numerical
indices in the interface. See
https://crrev.com/94dacdb8289038b7fa68c9f2bd57d5311b2cb5cb for details.

TBR=tommi@chromium.org

BUG=305720

Committed: https://crrev.com/143d02516f5a4459076f21bd9557a18638e657de
Cr-Commit-Position: refs/heads/master@{#389279}
==========

to

==========
Refactor ProfileInfoCache in most of c/b/profiles

ProfileInfoCache is being refactored into ProfileAttributesStorage and
ProfileAttributesEntry, which use profile paths instead of numerical
indices in the interface. See
https://crrev.com/94dacdb8289038b7fa68c9f2bd57d5311b2cb5cb for details.

TBR=tommi@chromium.org

BUG=305720

Committed: https://crrev.com/143d02516f5a4459076f21bd9557a18638e657de
Cr-Commit-Position: refs/heads/master@{#389279}
==========

[commit-bot: I haz the power, 2016-04-23 11:04:30]

Committed patchset #16 (id:340001)

[commit-bot: I haz the power, 2016-04-23 11:06:16]

Description was changed from

==========
Refactor ProfileInfoCache in most of c/b/profiles

ProfileInfoCache is being refactored into ProfileAttributesStorage and
ProfileAttributesEntry, which use profile paths instead of numerical
indices in the interface. See
https://crrev.com/94dacdb8289038b7fa68c9f2bd57d5311b2cb5cb for details.

TBR=tommi@chromium.org

BUG=305720

Committed: https://crrev.com/143d02516f5a4459076f21bd9557a18638e657de
Cr-Commit-Position: refs/heads/master@{#389279}
==========

to

==========
Refactor ProfileInfoCache in most of c/b/profiles

ProfileInfoCache is being refactored into ProfileAttributesStorage and
ProfileAttributesEntry, which use profile paths instead of numerical
indices in the interface. See
https://crrev.com/94dacdb8289038b7fa68c9f2bd57d5311b2cb5cb for details.

TBR=tommi@chromium.org

BUG=305720

Committed: https://crrev.com/143d02516f5a4459076f21bd9557a18638e657de
Cr-Commit-Position: refs/heads/master@{#389279}

Committed: https://crrev.com/498e924981ba4e85ea6c5bc5bcedeea655090743
Cr-Commit-Position: refs/heads/master@{#389376}
==========

[commit-bot: I haz the power, 2016-04-23 11:06:19]

Patchset 16 (id:??) landed as
https://crrev.com/498e924981ba4e85ea6c5bc5bcedeea655090743
Cr-Commit-Position: refs/heads/master@{#389376}