Componentize SigninIdAccountHelper.

Componentize SigninIdAccountHelper.

This CL eliminates //chrome dependencies from SigninAccountIdHelper. It
additionally adds an observer-like API to SigninClient and uses this API to
have ChromeSigninClient performs the LocalAuth functionality that SigninManager
was previously performing (LocalAuth will remain a //chrome-level concept).

BUG=354864, 354866
TBR=thakis

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

[blundell, 2014-03-21 15:01:46]

https://codereview.chromium.org/205703005/diff/1/chrome/browser/signin/chrome...
File chrome/browser/signin/chrome_signin_client.h (right):

https://codereview.chromium.org/205703005/diff/1/chrome/browser/signin/chrome...
chrome/browser/signin/chrome_signin_client.h:34: #if !defined(OS_CHROMEOS)
I decided to make this API non-ChromeOS to make it clear that these functions
wouldn't be called when only SigninManagerBase was used (to preserve the
previous behavior wrt SigninIdAccountHelper, I need to call these functions from
SigninManager rather than SigninManagerBase). It's a little ugly, but IIUC
SigninIdAccountHelper will be going away soon, at which point these functions
can go away too. Anyway, open to other ideas if you have them.

[Roger Tawa OOO till Jul 10th, 2014-03-24 14:10:07]

Concepts like SigninManagerInitialized() and SigninManagerShutdown() could be
useful to cros, so it does seem a little strange to restrict those to non-cros.

The SigninAccountIdHelper could also be compartmentalized by injecting SM, O2TS,
and the request context into it.  Not sure if there is a way to inject prefs
without dependencies on chrome, but if not then it might be better to pass a
pointer to SigninClient to SigninAccountIdHelper, who can then call a new method
SigninClient::SetAccountIdPref() or something.  Then we don't need
SigninManagerInitialized() and SigninManagerShutdown().

What do you think?

https://codereview.chromium.org/205703005/diff/1/chrome/browser/signin/signin...
File chrome/browser/signin/signin_manager.cc (right):

https://codereview.chromium.org/205703005/diff/1/chrome/browser/signin/signin...
chrome/browser/signin/signin_manager.cc:312:
client_->SigninManagerShutdown(this);
At this point, the SM is sort of like half shutdown.  Maybe better to move this
to above line 308, and maybe rename the function to something that indicates its
more like a pre-shutdon hook?  Like BeforeSigninManagerShutdown() ?

[blundell, 2014-03-26 16:12:22]

Hi Roger,

Acted on your suggestion to componentize SigninAccountIdHelper instead of
abstracting out of SigninManager. Much cleaner now I think -- PTAL. (Note that I
can't move SigninAccountIdHelper yet as it depends on SigninManager).

[Roger Tawa OOO till Jul 10th, 2014-03-27 15:53:29]

lgtm

Looks great, thanks Colin!

https://codereview.chromium.org/205703005/diff/20001/chrome/browser/signin/si...
File chrome/browser/signin/signin_account_id_helper.cc (right):

https://codereview.chromium.org/205703005/diff/20001/chrome/browser/signin/si...
chrome/browser/signin/signin_account_id_helper.cc:115: DCHECK(signin_manager_);
DCHECK client_ is not null?

[blundell, 2014-03-27 17:43:55]

https://codereview.chromium.org/205703005/diff/20001/chrome/browser/signin/si...
File chrome/browser/signin/signin_account_id_helper.cc (right):

https://codereview.chromium.org/205703005/diff/20001/chrome/browser/signin/si...
chrome/browser/signin/signin_account_id_helper.cc:115: DCHECK(signin_manager_);
On 2014/03/27 15:53:29, Roger Tawa wrote:
> DCHECK client_ is not null?

Done.

[blundell, 2014-03-27 17:44:27]

TBR thakis for //chrome/common

[blundell, 2014-03-27 17:44:41]

The CQ bit was checked by blundell@chromium.org

[commit-bot: I haz the power, 2014-03-27 17:45:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/blundell@chromium.org/205703005/40001

[commit-bot: I haz the power, 2014-03-27 18:58:40]

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

[commit-bot: I haz the power, 2014-03-27 18:58:40]

Retried try job too often on win_rel for step(s) content_browsertests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[blundell, 2014-03-27 20:36:45]

The CQ bit was checked by blundell@chromium.org

[commit-bot: I haz the power, 2014-03-27 20:37:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/blundell@chromium.org/205703005/40001

[commit-bot: I haz the power, 2014-03-27 22:27:10]

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

[commit-bot: I haz the power, 2014-03-27 22:27:10]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[blundell, 2014-03-28 06:31:48]

The CQ bit was checked by blundell@chromium.org

[commit-bot: I haz the power, 2014-03-28 06:32:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/blundell@chromium.org/205703005/40001

[commit-bot: I haz the power, 2014-03-28 06:35:33]

Change committed as 260080