Profile chooser on mac was passing wrong value to signin error controller.

Profile chooser on mac was passing wrong value to signin error controller.  At the same time, I noticed another case where an infinite spinner could happen on any platform, see fix in inline_login_handler_impl.cc.

Fixed MutableProfileOAuth2TokenService to canonicalize the primary account id given to it in order to properly support loading older profiles.  This could cause chrome to show an auth error even after a successful reauth.

Refactored auth error providers to provide only the account id.
Signin error controller should no longer care about username.

BUG=476739
Committed: https://crrev.com/7470d4c05962b89ada2647e657436a6d582cb677
Cr-Commit-Position: refs/heads/master@{#327307}

[Roger Tawa OOO till Jul 10th, 2015-04-21 21:55:21]

Patchset #1 (id:1) has been deleted

[Roger Tawa OOO till Jul 10th, 2015-04-23 15:13:44]

Patchset #1 (id:20001) has been deleted

[Roger Tawa OOO till Jul 10th, 2015-04-23 15:13:49]

Patchset #1 (id:40001) has been deleted

[Roger Tawa OOO till Jul 10th, 2015-04-23 15:13:57]

Patchset #1 (id:60001) has been deleted

[Roger Tawa OOO till Jul 10th, 2015-04-23 15:14:04]

Patchset #1 (id:80001) has been deleted

[Roger Tawa OOO till Jul 10th, 2015-04-24 18:09:29]

Patchset #1 (id:100001) has been deleted

[Roger Tawa OOO till Jul 10th, 2015-04-24 18:09:35]

Patchset #1 (id:120001) has been deleted

[Roger Tawa OOO till Jul 10th, 2015-04-24 18:09:41]

Patchset #1 (id:140001) has been deleted

[Roger Tawa OOO till Jul 10th, 2015-04-24 18:27:19]

Hi Mike,

Can you please take a look?  Thanks.

[Roger Tawa OOO till Jul 10th, 2015-04-24 18:27:45]

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

[Roger Tawa OOO till Jul 10th, 2015-04-24 18:27:45]

Hi Mike,

Can you please take a look?  Thanks.

[Mike Lerman, 2015-04-24 19:36:27]

Great refactor! Not actually such a big CL.

Two optional small yak shaves, if you think they also make sense. I'm not
passionate about either one.

https://codereview.chromium.org/1094103005/diff/160001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right):

https://codereview.chromium.org/1094103005/diff/160001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1999:
errorController = profiles::GetSigninErrorController(browser_->profile());
Would you be okay changing this to referencing the SigninErrorControllerFactory
instead of the method in profiles_state? It'd be great to move away from having
that helper method in profiles_state, now that we have a nice PKS. (both here
and in the ProfileChooserView)

https://codereview.chromium.org/1094103005/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right):

https://codereview.chromium.org/1094103005/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:122: std::string
GetAuthErrorAccountId(Profile* profile) {
nit: it's odd at that this a helper method, while profile_chooser_controller
uses an inline ternary operator. Not sure where there's a good place to put this
method either, unless it's profile_window.

If it didn't bother you, then it's not that important to move it.

https://codereview.chromium.org/1094103005/diff/160001/components/signin/core...
File components/signin/core/browser/about_signin_internals.cc (right):

https://codereview.chromium.org/1094103005/diff/160001/components/signin/core...
components/signin/core/browser/about_signin_internals.cc:549: std::string
error_account_id =
nit: Can this and just below be const std::string&?

https://codereview.chromium.org/1094103005/diff/160001/components/signin/core...
File components/signin/core/browser/signin_error_controller.cc (right):

https://codereview.chromium.org/1094103005/diff/160001/components/signin/core...
components/signin/core/browser/signin_error_controller.cc:7: #include
"components/signin/core/browser/account_tracker_service.h"
Is this include necessary?

[Roger Tawa OOO till Jul 10th, 2015-04-27 20:32:00]

Thanks Mike, please take another look.

https://codereview.chromium.org/1094103005/diff/160001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right):

https://codereview.chromium.org/1094103005/diff/160001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1999:
errorController = profiles::GetSigninErrorController(browser_->profile());
On 2015/04/24 19:36:26, Mike Lerman wrote:
> Would you be okay changing this to referencing the
SigninErrorControllerFactory
> instead of the method in profiles_state? It'd be great to move away from
having
> that helper method in profiles_state, now that we have a nice PKS. (both here
> and in the ProfileChooserView)

Done.

https://codereview.chromium.org/1094103005/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right):

https://codereview.chromium.org/1094103005/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:122: std::string
GetAuthErrorAccountId(Profile* profile) {
On 2015/04/24 19:36:26, Mike Lerman wrote:
> nit: it's odd at that this a helper method, while profile_chooser_controller
> uses an inline ternary operator. Not sure where there's a good place to put
this
> method either, unless it's profile_window.
> 
> If it didn't bother you, then it's not that important to move it.

Make helper function.

https://codereview.chromium.org/1094103005/diff/160001/components/signin/core...
File components/signin/core/browser/about_signin_internals.cc (right):

https://codereview.chromium.org/1094103005/diff/160001/components/signin/core...
components/signin/core/browser/about_signin_internals.cc:549: std::string
error_account_id =
On 2015/04/24 19:36:26, Mike Lerman wrote:
> nit: Can this and just below be const std::string&?

Done.

https://codereview.chromium.org/1094103005/diff/160001/components/signin/core...
File components/signin/core/browser/signin_error_controller.cc (right):

https://codereview.chromium.org/1094103005/diff/160001/components/signin/core...
components/signin/core/browser/signin_error_controller.cc:7: #include
"components/signin/core/browser/account_tracker_service.h"
On 2015/04/24 19:36:27, Mike Lerman wrote:
> Is this include necessary?

Oops, left over from old code.  Removed.

[Mike Lerman, 2015-04-27 20:38:21]

Great - thanks! LGTM.

[Roger Tawa OOO till Jul 10th, 2015-04-27 21:23:27]

rogerta@chromium.org changed reviewers:
+ avi@chromium.org, sky@chromium.org, zea@chromium.org

[Roger Tawa OOO till Jul 10th, 2015-04-27 21:23:27]

Hi Avi, Nicolas, Scott,

Can you please do owner review?

Avi:
app_controller_mac_unittest.mm
profile_chooser_controller.mm

Nicolas:
sync_ui_util_unittest.cc

Scott:
profile_chooser_view.cc

[sky, 2015-04-27 21:57:02]

profiler_chooser_view.cc LGTM

[Avi (use Gerrit), 2015-04-28 00:14:39]

app_controller_mac_unittest.mm lgtm

[Avi (use Gerrit), 2015-04-28 14:55:44]

profile_chooser_controller.mm lgtm too

I totally missed that line, sorry!

[Nicolas Zea, 2015-04-28 16:50:35]

sync lgtm

[Roger Tawa OOO till Jul 10th, 2015-04-28 16:54:02]

The CQ bit was checked by rogerta@chromium.org

[Roger Tawa OOO till Jul 10th, 2015-04-28 16:54:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlerman@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1094103005/#ps240001
(title: "rebased")

[commit-bot: I haz the power, 2015-04-28 16:54:36]

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

[commit-bot: I haz the power, 2015-04-28 17:00:45]

Committed patchset #5 (id:240001)

[commit-bot: I haz the power, 2015-04-28 17:01:48]

Patchset 5 (id:??) landed as
https://crrev.com/7470d4c05962b89ada2647e657436a6d582cb677
Cr-Commit-Position: refs/heads/master@{#327307}