Move auth-error reporting code out of SyncGlobalError and into SigninGlobalError

Move auth-error reporting code out of SyncGlobalError and into SigninGlobalError

This is the first step towards enabling signin even when sync is disabled by
policy.

BUG=166148, 113133
Relanding after rollout due to failures in ExtensionActionContextMenuTest.BrowserAction

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

[Andrew T Wilson (Slow), 2013-01-11 13:48:40]

Roger, PTAL at browser/signin
Sky, PTAL at browser/ui/toolbar/wrench_menu_model.*
Tim, PTAL at browser/sync

[sky, 2013-01-11 17:03:42]

LGTM

[Roger Tawa OOO till Jul 10th, 2013-01-14 16:06:51]

lgtm

https://codereview.chromium.org/11817045/diff/18001/chrome/browser/signin/sig...
File chrome/browser/signin/signin_manager.cc (right):

https://codereview.chromium.org/11817045/diff/18001/chrome/browser/signin/sig...
chrome/browser/signin/signin_manager.cc:116: }
I think we should keep the order of function definition in the cc file the same
as the order of function declaration in the h  file.

https://codereview.chromium.org/11817045/diff/18001/chrome/browser/sync/profi...
File chrome/browser/sync/profile_sync_service_harness.cc (right):

https://codereview.chromium.org/11817045/diff/18001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service_harness.cc:127: }
How is this affected by the rest of this CL?

[tim (not reviewing), 2013-01-15 04:25:42]

https://codereview.chromium.org/11817045/diff/18001/chrome/browser/sync/profi...
File chrome/browser/sync/profile_sync_service.h (right):

https://codereview.chromium.org/11817045/diff/18001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service.h:408: virtual GoogleServiceAuthError
GetAuthStatus() const OVERRIDE;
Hmm.. we have this and GetAuthError() on this class now :/  How does one tell
the difference?  Can we maybe add more detail to the name of this method in the
interface?

https://codereview.chromium.org/11817045/diff/18001/chrome/browser/sync/profi...
File chrome/browser/sync/profile_sync_service_unittest.cc (right):

https://codereview.chromium.org/11817045/diff/18001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service_unittest.cc:253:
harness_.service->Shutdown();
... what was going on here before?  Isn't there a CHECK in ~PSS that Shutdown
was called?

https://codereview.chromium.org/11817045/diff/18001/chrome/browser/sync/sync_...
File chrome/browser/sync/sync_global_error.cc (left):

https://codereview.chromium.org/11817045/diff/18001/chrome/browser/sync/sync_...
chrome/browser/sync/sync_global_error.cc:63: if
(service_->GetAuthError().state() != AuthError::NONE) {
Yay! removal of ifdef!

[Andrew T Wilson (Slow), 2013-01-15 13:24:56]

Tim, got a question for you about GetAuthStatus vs GetAuthError.

https://codereview.chromium.org/11817045/diff/18001/chrome/browser/signin/sig...
File chrome/browser/signin/signin_manager.cc (right):

https://codereview.chromium.org/11817045/diff/18001/chrome/browser/signin/sig...
chrome/browser/signin/signin_manager.cc:116: }
On 2013/01/14 16:06:52, Roger Tawa wrote:
> I think we should keep the order of function definition in the cc file the
same
> as the order of function declaration in the h  file.

Done.

https://codereview.chromium.org/11817045/diff/18001/chrome/browser/sync/profi...
File chrome/browser/sync/profile_sync_service.h (right):

https://codereview.chromium.org/11817045/diff/18001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service.h:408: virtual GoogleServiceAuthError
GetAuthStatus() const OVERRIDE;
On 2013/01/15 04:25:42, timsteele wrote:
> Hmm.. we have this and GetAuthError() on this class now :/  How does one tell
> the difference?  Can we maybe add more detail to the name of this method in
the
> interface?

In general, I don't think that the existence of a similar API in a downstream
class should impact the naming of an upstream API. I don't think that
AuthStatusProvider::GetAuthStatus() is unclear, in and of itself (I think it's
pretty clear that it gets the current authenticated status of that provider and
if you were reviewing that API in the absence of your knowledge of
ProfileSyncService you'd have no objections), so I'm not sure why we would
rename it.

That said, if you have a suggestion you find acceptable, I'm happy to rename it
- just let me know. I'm not too concerned about confusion since GetAuthStatus()
and GetAuthError() are basically interchangeable anyway.

Another alternative is for me to get rid of GetAuthError() entirely.

Anyhow, let me know what you'd like to do here.

https://codereview.chromium.org/11817045/diff/18001/chrome/browser/sync/profi...
File chrome/browser/sync/profile_sync_service_harness.cc (right):

https://codereview.chromium.org/11817045/diff/18001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service_harness.cc:127: }
On 2013/01/14 16:06:52, Roger Tawa wrote:
> How is this affected by the rest of this CL?

The problem is ProfileSyncService::ShutdownImpl() can now result in notifying
observers, in the case that there's an auth error (this is a side-effect of
calling UpdateAuthError() to clear our auth error if sync is disabled/signed
out).

ProfileSyncServiceHarness is not always careful to remove itself as an observer
(it adds itself in the constructor, and doesn't remove itself) - this would lead
to crashes in tests when ProfileSyncService notified observers during shutdown.
So I added this to ProfileSyncServiceHarness so it doesn't leave dangling
references to itself in the observer list any longer.

https://codereview.chromium.org/11817045/diff/18001/chrome/browser/sync/profi...
File chrome/browser/sync/profile_sync_service_unittest.cc (right):

https://codereview.chromium.org/11817045/diff/18001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service_unittest.cc:253:
harness_.service->Shutdown();
On 2013/01/15 04:25:42, timsteele wrote:
> ... what was going on here before?  Isn't there a CHECK in ~PSS that Shutdown
> was called?
Nope. There was an implicit expectation that if you call PSS::Initialize() that
you'd then call PSS::Shutdown() before the object was freed, but there's nothing
enforcing that expectation.

Until now, that is - since PSS::Shutdown() now calls
SyncGlobalError::RemoveProvider(), freeing the PSS without calling Shutdown()
first leads to a DCHECK.

I had two ways to fix this - I could either add some kind of cleanup code to the
PSS destructor, or I could fix the tests to properly call Shutdown() and I chose
to do the right thing and fix the tests.

https://codereview.chromium.org/11817045/diff/18001/chrome/browser/sync/sync_...
File chrome/browser/sync/sync_global_error.cc (left):

https://codereview.chromium.org/11817045/diff/18001/chrome/browser/sync/sync_...
chrome/browser/sync/sync_global_error.cc:63: if
(service_->GetAuthError().state() != AuthError::NONE) {
On 2013/01/15 04:25:42, timsteele wrote:
> Yay! removal of ifdef!

Sadly, it's not a removal, it's just moved to a different file :( We still have
to do something special here on chromeos since you can only update auth
credentials by going through the signin flow again (i.e. exit the browser).

[Andrew T Wilson (Slow), 2013-01-16 08:59:32]

I'm going to land this as-is. Tim, let me know if you have suggestions for
resolving GetAuthError vs GetAuthStatus and I can address in a follow-on patch.

[commit-bot: I haz the power, 2013-01-16 08:59:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/atwilson@chromium.org/11817045/47001

[commit-bot: I haz the power, 2013-01-16 13:10:57]

Change committed as 177136