Refactor the code that creates the list of secondary accounts

Refactor the code that creates the list of secondary accounts

BUG=331805
TEST=No visible change. Existing unit tests.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=243712

[noms (inactive), 2014-01-08 16:50:27]

Hi Rachel,

I did a small refactor to common code used by the views/mac avatar bubbles.
Please take a look. Thanks!

[rpetterson, 2014-01-08 18:12:18]

LGTM with one nit.

[rpetterson, 2014-01-08 18:12:32]

Eh, here's the nit :)

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

https://codereview.chromium.org/128513002/diff/60001/chrome/browser/profiles/...
chrome/browser/profiles/profiles_state.cc:117: accounts.erase(primary_index);
You could do it one go with:
accounts.erase(std::remove_if(accounts.begin(),
                              accounts.end(),
                              std::bind1st(std::equal_to<std::string>(),
primary_account)),
               accounts.end());

Unless you need to verify that the primary account was previously in the list,
in which case your version does that with the DCHECK.

[noms (inactive), 2014-01-08 19:13:21]

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

https://codereview.chromium.org/128513002/diff/60001/chrome/browser/profiles/...
chrome/browser/profiles/profiles_state.cc:117: accounts.erase(primary_index);
The original code also explicitly DCHECK'ed that the primary account was in the
list, as if it isn't, something has gone very, very bad, and the TokenService is
out of sync with the SigninManager (which knows about the primary account). 

 On 2014/01/08 18:12:33, rpetterson wrote:
> You could do it one go with:
> accounts.erase(std::remove_if(accounts.begin(),
>                               accounts.end(),
>                               std::bind1st(std::equal_to<std::string>(),
> primary_account)),
>                accounts.end());
> 
> Unless you need to verify that the primary account was previously in the list,
> in which case your version does that with the DCHECK.

[noms (inactive), 2014-01-08 19:16:34]

Hi Scott and Alexei,

This CL does a pretty minor refactor and pulls some common views/mac code in a
utility function. Would you mind owner stamping it, please?

Scott: chrome/browser/ui/views/profile_chooser_view.cc
Alexei: chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm

Thanks!

[sky, 2014-01-08 21:00:15]

LGTM

https://codereview.chromium.org/128513002/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/profile_chooser_view.cc (right):

https://codereview.chromium.org/128513002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/profile_chooser_view.cc:801: for (size_t i = 0; i <
accounts.size(); ++i) {
nit: remove {}

[noms (inactive), 2014-01-08 21:04:27]

https://codereview.chromium.org/128513002/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/profile_chooser_view.cc (right):

https://codereview.chromium.org/128513002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/profile_chooser_view.cc:801: for (size_t i = 0; i <
accounts.size(); ++i) {
Eeek, oops. Done.
On 2014/01/08 21:00:15, sky wrote:
> nit: remove {}

[Alexei Svitkine (slow), 2014-01-08 21:28:37]

https://codereview.chromium.org/128513002/diff/200001/chrome/browser/profiles...
File chrome/browser/profiles/profiles_state.cc (right):

https://codereview.chromium.org/128513002/diff/200001/chrome/browser/profiles...
chrome/browser/profiles/profiles_state.cc:107: Profile* profile, const
std::string& primary_account) {
Nit: 1 param per line if your first param is on a new line, per style guide.

https://codereview.chromium.org/128513002/diff/200001/chrome/browser/profiles...
File chrome/browser/profiles/profiles_state.h (right):

https://codereview.chromium.org/128513002/diff/200001/chrome/browser/profiles...
chrome/browser/profiles/profiles_state.h:41: // the |primary_account|.
Nit: Mention what these strings are - e.g. email addresses?

https://codereview.chromium.org/128513002/diff/200001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm (right):

https://codereview.chromium.org/128513002/diff/200001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:605: // from the
others, so more work is likely required here: crbug.com/311124.
Should this TODO be in the utility function? Or does this mean differentiate in
the UI? If so, please clarify the comment.

[noms (inactive), 2014-01-08 21:35:13]

https://codereview.chromium.org/128513002/diff/200001/chrome/browser/profiles...
File chrome/browser/profiles/profiles_state.cc (right):

https://codereview.chromium.org/128513002/diff/200001/chrome/browser/profiles...
chrome/browser/profiles/profiles_state.cc:107: Profile* profile, const
std::string& primary_account) {
On 2014/01/08 21:28:37, Alexei Svitkine wrote:
> Nit: 1 param per line if your first param is on a new line, per style guide.

Done.

https://codereview.chromium.org/128513002/diff/200001/chrome/browser/profiles...
File chrome/browser/profiles/profiles_state.h (right):

https://codereview.chromium.org/128513002/diff/200001/chrome/browser/profiles...
chrome/browser/profiles/profiles_state.h:41: // the |primary_account|.
On 2014/01/08 21:28:37, Alexei Svitkine wrote:
> Nit: Mention what these strings are - e.g. email addresses?

Done.

https://codereview.chromium.org/128513002/diff/200001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm (right):

https://codereview.chromium.org/128513002/diff/200001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:605: // from the
others, so more work is likely required here: crbug.com/311124.
Differentiate in the UI. Fixed.
On 2014/01/08 21:28:37, Alexei Svitkine wrote:
> Should this TODO be in the utility function? Or does this mean differentiate
in
> the UI? If so, please clarify the comment.

[Alexei Svitkine (slow), 2014-01-08 21:44:51]

lgtm thanks

[commit-bot: I haz the power, 2014-01-08 21:51:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/noms@chromium.org/128513002/270001

[commit-bot: I haz the power, 2014-01-09 00:41:18]

Change committed as 243712