Refactor fetching profile avatar icons and labels repeated md-settings & md-user-manager (C++)

Refactor fetching profile avatar icons and labels repeated md-settings & md-user-manager (C++)

Committed: https://crrev.com/b11ebce75c8d73b26948aa4e77b51b2dbfab1774
Cr-Commit-Position: refs/heads/master@{#401041}

[Moe, 2016-06-15 21:14:33]

mahmadi@chromium.org changed reviewers:
+ tommycli@chromium.org

[Moe, 2016-06-15 21:14:34]

Hi Tommy, this is the second part of the refactor. Please take a look.

[tommycli, 2016-06-15 21:19:35]

This is LGTM thanks!

[tommycli, 2016-06-15 21:22:44]

I got overly excited. I do some some nits below, but it's still LGTM sans these
nitpicks.

https://codereview.chromium.org/2066093003/diff/1/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_avatar_icon_util.cc (right):

https://codereview.chromium.org/2066093003/diff/1/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_avatar_icon_util.cc:500: // Add the default
avatar icons and labels.
nit: Can axe this comment since it's now within the function name

https://codereview.chromium.org/2066093003/diff/1/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_avatar_icon_util.cc:502: for (size_t i = 0; i <
profiles::GetDefaultAvatarIconCount() &&
Is this the preferred formatting from 'git cl format'? It seems odd to me

https://codereview.chromium.org/2066093003/diff/1/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_avatar_icon_util.h (right):

https://codereview.chromium.org/2066093003/diff/1/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_avatar_icon_util.h:113:
std::unique_ptr<base::ListValue> GetDefaultProfileAvatarIconsAndLabels();
nit: Can ListValue be forward declared here and values.h imported into the .cc
file?

[Moe, 2016-06-16 12:52:51]

Patchset #2 (id:20001) has been deleted

[Moe, 2016-06-16 13:36:46]

mahmadi@chromium.org changed reviewers:
+ mlerman@chromium.org

[Moe, 2016-06-16 13:36:46]

Thanks Tommy. Mike, Could you please review this CL?

https://codereview.chromium.org/2066093003/diff/1/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_avatar_icon_util.cc (right):

https://codereview.chromium.org/2066093003/diff/1/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_avatar_icon_util.cc:500: // Add the default
avatar icons and labels.
On 2016/06/15 21:22:44, tommycli wrote:
> nit: Can axe this comment since it's now within the function name

Done.

https://codereview.chromium.org/2066093003/diff/1/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_avatar_icon_util.cc:502: for (size_t i = 0; i <
profiles::GetDefaultAvatarIconCount() &&
On 2016/06/15 21:22:44, tommycli wrote:
> Is this the preferred formatting from 'git cl format'? It seems odd to me

yep. It doesn't complain.

https://codereview.chromium.org/2066093003/diff/1/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_avatar_icon_util.h (right):

https://codereview.chromium.org/2066093003/diff/1/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_avatar_icon_util.h:113:
std::unique_ptr<base::ListValue> GetDefaultProfileAvatarIconsAndLabels();
On 2016/06/15 21:22:44, tommycli wrote:
> nit: Can ListValue be forward declared here and values.h imported into the .cc
> file?

Good point. Done.

[Mike Lerman, 2016-06-20 00:42:54]

Sorry for the slow response, was OOO.

LGTM % comments

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

https://codereview.chromium.org/2066093003/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profile_avatar_icon_util.cc:501: size_t
placeholder_avatar_index = profiles::GetPlaceholderAvatarIndex();
nit: const

https://codereview.chromium.org/2066093003/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profile_avatar_icon_util.cc:503: i !=
placeholder_avatar_index;
nit: ++i

https://codereview.chromium.org/2066093003/diff/40001/chrome/browser/profiles...
File chrome/browser/profiles/profile_avatar_icon_util.h (right):

https://codereview.chromium.org/2066093003/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profile_avatar_icon_util.h:113: // labels.
Please note how this list is ordered.
Also note - what label? I assume this is the Profile name for display? Can
probably just call this the Avatar name, and such call the method IconsAndNames.

[Moe, 2016-06-20 17:24:08]

Thank you Mike!

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

https://codereview.chromium.org/2066093003/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profile_avatar_icon_util.cc:501: size_t
placeholder_avatar_index = profiles::GetPlaceholderAvatarIndex();
On 2016/06/20 00:42:53, Mike Lerman wrote:
> nit: const

Done.

https://codereview.chromium.org/2066093003/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profile_avatar_icon_util.cc:503: i !=
placeholder_avatar_index;
On 2016/06/20 00:42:53, Mike Lerman wrote:
> nit: ++i

Done.

https://codereview.chromium.org/2066093003/diff/40001/chrome/browser/profiles...
File chrome/browser/profiles/profile_avatar_icon_util.h (right):

https://codereview.chromium.org/2066093003/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profile_avatar_icon_util.h:113: // labels.
On 2016/06/20 00:42:54, Mike Lerman wrote:
> Please note how this list is ordered.
> Also note - what label? I assume this is the Profile name for display? Can
> probably just call this the Avatar name, and such call the method
IconsAndNames.

a. I amended the comment to note what the labels are and how the list is
ordered.
b. These 'labels' more describe the avatars for a11y. I chose the word 'label'
to avoid confusion with the legacy avatar names (that we don't use anymore
afaik).
https://cs.chromium.org/chromium/src/chrome/app/generated_resources.grd?l=10895
Maybe 'title' would've been a better choice. let's keep using the word 'label'
here for consistency with the resource IDs?

[Mike Lerman, 2016-06-20 17:38:02]

sLGTM

https://codereview.chromium.org/2066093003/diff/40001/chrome/browser/profiles...
File chrome/browser/profiles/profile_avatar_icon_util.h (right):

https://codereview.chromium.org/2066093003/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profile_avatar_icon_util.h:113: // labels.
On 2016/06/20 17:24:08, Moe wrote:
> On 2016/06/20 00:42:54, Mike Lerman wrote:
> > Please note how this list is ordered.
> > Also note - what label? I assume this is the Profile name for display? Can
> > probably just call this the Avatar name, and such call the method
> IconsAndNames.
> 
> a. I amended the comment to note what the labels are and how the list is
> ordered.
> b. These 'labels' more describe the avatars for a11y. I chose the word 'label'
> to avoid confusion with the legacy avatar names (that we don't use anymore
> afaik).
>
https://cs.chromium.org/chromium/src/chrome/app/generated_resources.grd?l=10895
> Maybe 'title' would've been a better choice. let's keep using the word 'label'
> here for consistency with the resource IDs?

Ah, those labels used to also be the default profile name, but that doesn't seem
to be the case anymore. Label is fine then :)

[Moe, 2016-06-21 17:53:41]

The CQ bit was checked by mahmadi@chromium.org

[Moe, 2016-06-21 17:53:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
tommycli@chromium.org, mlerman@chromium.org
Link to the patchset: https://codereview.chromium.org/2066093003/#ps70001
(title: "rebase")

[commit-bot: I haz the power, 2016-06-21 17:54:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2066093003/70001

[Moe, 2016-06-21 17:55:49]

On 2016/06/21 17:54:20, commit-bot: I haz the power wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/2066093003/70001

Thank you.

[commit-bot: I haz the power, 2016-06-21 18:00:19]

Committed patchset #4 (id:70001)

[commit-bot: I haz the power, 2016-06-21 18:16:32]

Description was changed from

==========
Refactor fetching profile avatar icons and labels repeated md-settings &
md-user-manager (C++)
==========

to

==========
Refactor fetching profile avatar icons and labels repeated md-settings &
md-user-manager (C++)

Committed: https://crrev.com/b11ebce75c8d73b26948aa4e77b51b2dbfab1774
Cr-Commit-Position: refs/heads/master@{#401041}
==========

[commit-bot: I haz the power, 2016-06-21 18:16:33]

Patchset 4 (id:??) landed as
https://crrev.com/b11ebce75c8d73b26948aa4e77b51b2dbfab1774
Cr-Commit-Position: refs/heads/master@{#401041}