Make the SigninErrorController a PKS.

Make the SigninErrorController a PKS.

This is a prelude to https://codereview.chromium.org/813133003/, which will introduce some dependencies between the SigninErrorController and some other PKSes. To make these cleaner, this is a yak shave to move the SigninErrorController out from being a member of the ProfileOAuth2TokenService (which is a ProfileKeyedService) and into its own PKS.

BUG=434829
Committed: https://crrev.com/3247f076b4c8b9be5281d34b217c9236688a6725
Cr-Commit-Position: refs/heads/master@{#310774}

[Mike Lerman, 2015-01-08 15:53:14]

mlerman@chromium.org changed reviewers:
+ estade@chromium.org, noms@chromium.org, pavely@chromium.org,
rogerta@chromium.org, rsesek@chromium.org

[Mike Lerman, 2015-01-08 15:53:15]

Hi all,

Outside of signin this is mostly a refactoring CL. Please reviews.

rsesek - c/b/ui/cocoa
noms - c/b/profiles
estade - c/b/ui/webui/options
pavely - c/b/sync
rogerta - c/b/signin and components/signin

Thanks!

[pavely, 2015-01-08 19:46:07]

c/b/sync lgtm

[noms (inactive), 2015-01-08 21:16:40]

profiles lgtm. Whew, this was an easy one.

[Evan Stade, 2015-01-08 21:44:51]

webui lgtm

[Robert Sesek, 2015-01-08 21:51:31]

cocoa/ LGTM

[Roger Tawa OOO till Jul 10th, 2015-01-08 22:30:54]

https://codereview.chromium.org/809313009/diff/80001/chrome/browser/signin/si...
File chrome/browser/signin/signin_error_notifier_ash_unittest.cc (right):

https://codereview.chromium.org/809313009/diff/80001/chrome/browser/signin/si...
chrome/browser/signin/signin_error_notifier_ash_unittest.cc:20: #include
"components/signin/core/browser/profile_oauth2_token_service.h"
I think you can remove this header and the factory too.

https://codereview.chromium.org/809313009/diff/80001/components/signin/core/b...
File components/signin/core/browser/profile_oauth2_token_service.h (right):

https://codereview.chromium.org/809313009/diff/80001/components/signin/core/b...
components/signin/core/browser/profile_oauth2_token_service.h:44:
SigninErrorController* signin_error_controller);
Instead of adding a signin_error_controller argument here, I think it would be
better to add it to ChromeSigninClient.  The base class SigninClient can expose
a signin_error_controller() method, and the ChromeSigninClient factory can set
the SigninErrorController.  I think this would simplify PO2TS and its derived
classes and minimize the coupling.

[Mike Lerman, 2015-01-09 14:27:08]

https://codereview.chromium.org/809313009/diff/80001/chrome/browser/signin/si...
File chrome/browser/signin/signin_error_notifier_ash_unittest.cc (right):

https://codereview.chromium.org/809313009/diff/80001/chrome/browser/signin/si...
chrome/browser/signin/signin_error_notifier_ash_unittest.cc:20: #include
"components/signin/core/browser/profile_oauth2_token_service.h"
On 2015/01/08 22:30:54, Roger Tawa wrote:
> I think you can remove this header and the factory too.

Removed the PO2TS header. Factory was already removed.

https://codereview.chromium.org/809313009/diff/80001/components/signin/core/b...
File components/signin/core/browser/profile_oauth2_token_service.h (right):

https://codereview.chromium.org/809313009/diff/80001/components/signin/core/b...
components/signin/core/browser/profile_oauth2_token_service.h:44:
SigninErrorController* signin_error_controller);
On 2015/01/08 22:30:54, Roger Tawa wrote:
> Instead of adding a signin_error_controller argument here, I think it would be
> better to add it to ChromeSigninClient.  The base class SigninClient can
expose
> a signin_error_controller() method, and the ChromeSigninClient factory can set
> the SigninErrorController.  I think this would simplify PO2TS and its derived
> classes and minimize the coupling.

I disagree, for a few reasons. 

The SigninClient is supposed to expose things outside the component (from
chrome/), but the SigninErrorController's in the component. 

In the CL to introduce the Auth Error Histogram, I'll be adding a dependency
from the SigninErrorController onto the SigninClient, so that the
SigninErrorController can set things on the ProfileInfoCache. If I here add an
unnecessary dependency from the SigninClient to the SigninErrorController, I'll
now have circular dependencies :S

I don't like hiding the dependency from PO2TS->SigninErrorController. There's a
real dependency (all the subclasses rely on it), which I think is best made
obvious in the factory, which is how the PKS dependencies should work. All we'd
be doing is passing the link from PO2TS to SigninClient (which doesn't currently
depend on SigninErrorController), which then makes everything that depends on
SigninClient now also dependent on SigninErrorController needlessly.

[Roger Tawa OOO till Jul 10th, 2015-01-09 15:31:03]

lgtm

OK sounds good Mike.

[Mike Lerman, 2015-01-09 15:31:38]

The CQ bit was checked by mlerman@chromium.org

[commit-bot: I haz the power, 2015-01-09 15:32:16]

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

[commit-bot: I haz the power, 2015-01-09 16:24:07]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-01-09 16:24:49]

Patchset 6 (id:??) landed as
https://crrev.com/3247f076b4c8b9be5281d34b217c9236688a6725
Cr-Commit-Position: refs/heads/master@{#310774}