Client changes for disabled dasher account

Client changes for disabled dasher account

When sync receives DISABLED_BY_ADMIN from server PSS stops SyncBackendHost and 
disables sync by settings DisabledByAdmin pref. This prevents sync from 
connecting to server in the future. The way to reenable sync is to sign out and 
sign in again.
In future milestone we will implement retry logic that will periodically 
(once a day) try to connect to sync server to see if sync is still disabled.

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

[tim (not reviewing), 2013-05-13 04:18:57]

https://codereview.chromium.org/14655009/diff/1/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/14655009/diff/1/chrome/common/pref_names.cc#n...
chrome/common/pref_names.cc:1801: const char kSyncDisabledByAdmin[] =
"sync.disabled_by_admin";
Can eventually replace sync.managed with this?  If so, can you add a TODO here
and reference a bug#?

https://codereview.chromium.org/14655009/diff/1/sync/engine/syncer_proto_util.cc
File sync/engine/syncer_proto_util.cc (right):

https://codereview.chromium.org/14655009/diff/1/sync/engine/syncer_proto_util...
sync/engine/syncer_proto_util.cc:347: error.action =
STOP_SYNC_FOR_DISABLED_ACCOUNT;
nit - indent should be two spaces from 'if' (345 was incorrect).

https://codereview.chromium.org/14655009/diff/1/sync/engine/syncer_proto_util...
sync/engine/syncer_proto_util.cc:409: // error_code =
sync_pb::SyncEnums::DISABLED_BY_ADMIN;
This needs to be fixed / cleaned up.

https://codereview.chromium.org/14655009/diff/1/sync/protocol/sync_protocol_e...
File sync/protocol/sync_protocol_error.h (right):

https://codereview.chromium.org/14655009/diff/1/sync/protocol/sync_protocol_e...
sync/protocol/sync_protocol_error.h:67: STOP_SYNC_FOR_DISABLED_ACCOUNT,
Why is this here as a ClientAction? It isn't part of the actionable error proto
and I'm not sure it really fits, since it's a lot more specific than these...

[Andrew T Wilson (Slow), 2013-05-13 11:51:13]

My main concern is that I don't think we want to keep the disabled status in a
pref - it should live in ProfileSyncService, in-memory.

Is there a reason why you made it a pref?

https://codereview.chromium.org/14655009/diff/1/chrome/browser/sync/profile_s...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/14655009/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service.cc:678: start_up_time_ = base::Time();
Why is this here?

https://codereview.chromium.org/14655009/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service.cc:1174: // user signs out and signs in
again.
We need to handle situations where the user cannot sign out (such as ChromeOS,
and desktop enterprise users).

https://codereview.chromium.org/14655009/diff/1/chrome/browser/sync/sync_prefs.h
File chrome/browser/sync/sync_prefs.h (right):

https://codereview.chromium.org/14655009/diff/1/chrome/browser/sync/sync_pref...
chrome/browser/sync/sync_prefs.h:99: bool IsSyncDisabledByAdmin() const;
See my previous comment - I don't think this should be kept in prefs. In any
case, if we keep it, we should document  what this means (how this is different
from IsManaged())

https://codereview.chromium.org/14655009/diff/1/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/14655009/diff/1/chrome/common/pref_names.cc#n...
chrome/common/pref_names.cc:1801: const char kSyncDisabledByAdmin[] =
"sync.disabled_by_admin";
I don't think this is the right way to approach this - we should not be storing
permanent client-side state that the user has disabled sync on the server.

This is especially true because on some platforms (ChromeOS, enterprise desktop)
you can't sign out. A better solution is to keep some flag in memory in PSS that
says that sync was disabled by the server, which is cleared on restart of
Chrome.

I thought that was the approach we were going with from our previous discussions
- is a permanent pref required for some reason?

https://codereview.chromium.org/14655009/diff/1/sync/protocol/sync_enums.proto
File sync/protocol/sync_enums.proto (right):

https://codereview.chromium.org/14655009/diff/1/sync/protocol/sync_enums.prot...
sync/protocol/sync_enums.proto:90: DISABLED_BY_ADMIN = 10;   // An administrator
disabled the account.
I think a more precise comment would be something like "An administrator has
disabled sync for this domain".

https://codereview.chromium.org/14655009/diff/1/sync/protocol/sync_protocol_e...
File sync/protocol/sync_protocol_error.h (right):

https://codereview.chromium.org/14655009/diff/1/sync/protocol/sync_protocol_e...
sync/protocol/sync_protocol_error.h:42: // An administrator disabled the
account.
See my previous comment - I think "disabled the account" is a bit vague, since
the admin is really disabling the sync service on the domain, not actually
disabling the user acct.

[pavely, 2013-05-13 22:37:08]

https://codereview.chromium.org/14655009/diff/1/chrome/browser/sync/profile_s...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/14655009/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service.cc:678: start_up_time_ = base::Time();
On 2013/05/13 11:51:13, Andrew T Wilson wrote:
> Why is this here?
PSS::StartUp checks that start_up_time_ is null. It is set to current time in
that function and then gets reset in ProfileSyncService::OnBackendInitialized()
for disabled account RPC to server will never succeed and therefore backend will
never get initialized. It caused failures in my early tests.

https://codereview.chromium.org/14655009/diff/1/chrome/browser/sync/sync_prefs.h
File chrome/browser/sync/sync_prefs.h (right):

https://codereview.chromium.org/14655009/diff/1/chrome/browser/sync/sync_pref...
chrome/browser/sync/sync_prefs.h:99: bool IsSyncDisabledByAdmin() const;
On 2013/05/13 11:51:13, Andrew T Wilson wrote:
> See my previous comment - I don't think this should be kept in prefs. In any
> case, if we keep it, we should document  what this means (how this is
different
> from IsManaged())

Done.

https://codereview.chromium.org/14655009/diff/1/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/14655009/diff/1/chrome/common/pref_names.cc#n...
chrome/common/pref_names.cc:1801: const char kSyncDisabledByAdmin[] =
"sync.disabled_by_admin";
On 2013/05/13 11:51:13, Andrew T Wilson wrote:
> I don't think this is the right way to approach this - we should not be
storing
> permanent client-side state that the user has disabled sync on the server.
> 
> This is especially true because on some platforms (ChromeOS, enterprise
desktop)
> you can't sign out. A better solution is to keep some flag in memory in PSS
that
> says that sync was disabled by the server, which is cleared on restart of
> Chrome.
> 
> I thought that was the approach we were going with from our previous
discussions
> - is a permanent pref required for some reason?
Flag is stored in prefs to display settings page correctly. Currently values
that affect states of buttons on settings page are kept in prefs and don't
require sync backend to be initialized and attempted to talk to server. I tried
to implement ProfileSyncService to talk to service right after initialization,
but that would mean issuing RPC every time right after browser startup. I think
we shouldn't be doing this and therefore I decided to cache this flag in prefs.
I'll work on the change where we will periodically (infrequently) initiallize
backend to check if account is still disabled.

https://codereview.chromium.org/14655009/diff/1/chrome/common/pref_names.cc#n...
chrome/common/pref_names.cc:1801: const char kSyncDisabledByAdmin[] =
"sync.disabled_by_admin";
On 2013/05/13 04:18:58, timsteele wrote:
> Can eventually replace sync.managed with this?  If so, can you add a TODO here
> and reference a bug#?
We can replace sync.managed once controlling IsManaged flag through group policy
on windows is deprecated. I don't know if/when this is planned. Will add TODO.

https://codereview.chromium.org/14655009/diff/1/sync/engine/syncer_proto_util.cc
File sync/engine/syncer_proto_util.cc (right):

https://codereview.chromium.org/14655009/diff/1/sync/engine/syncer_proto_util...
sync/engine/syncer_proto_util.cc:347: error.action =
STOP_SYNC_FOR_DISABLED_ACCOUNT;
On 2013/05/13 04:18:58, timsteele wrote:
> nit - indent should be two spaces from 'if' (345 was incorrect).

Done.

https://codereview.chromium.org/14655009/diff/1/sync/engine/syncer_proto_util...
sync/engine/syncer_proto_util.cc:409: // error_code =
sync_pb::SyncEnums::DISABLED_BY_ADMIN;
On 2013/05/13 04:18:58, timsteele wrote:
> This needs to be fixed / cleaned up.

Done.

https://codereview.chromium.org/14655009/diff/1/sync/protocol/sync_enums.proto
File sync/protocol/sync_enums.proto (right):

https://codereview.chromium.org/14655009/diff/1/sync/protocol/sync_enums.prot...
sync/protocol/sync_enums.proto:90: DISABLED_BY_ADMIN = 10;   // An administrator
disabled the account.
On 2013/05/13 11:51:13, Andrew T Wilson wrote:
> I think a more precise comment would be something like "An administrator has
> disabled sync for this domain".

Done.

https://codereview.chromium.org/14655009/diff/1/sync/protocol/sync_protocol_e...
File sync/protocol/sync_protocol_error.h (right):

https://codereview.chromium.org/14655009/diff/1/sync/protocol/sync_protocol_e...
sync/protocol/sync_protocol_error.h:42: // An administrator disabled the
account.
On 2013/05/13 11:51:13, Andrew T Wilson wrote:
> See my previous comment - I think "disabled the account" is a bit vague, since
> the admin is really disabling the sync service on the domain, not actually
> disabling the user acct.

Done.

https://codereview.chromium.org/14655009/diff/1/sync/protocol/sync_protocol_e...
sync/protocol/sync_protocol_error.h:67: STOP_SYNC_FOR_DISABLED_ACCOUNT,
On 2013/05/13 04:18:58, timsteele wrote:
> Why is this here as a ClientAction? It isn't part of the actionable error
proto
> and I'm not sure it really fits, since it's a lot more specific than these...
I followed pattern of DISABLE_SYNC_ON_CLIENT. It is not really an action sent by
server but triggered by client based on server's error code. See
syncer_proto_util.cc: ConvertLegacyErrorCodeToNewError.
It is needed because error is detected on sync thread but action should be taken
on browser thread and ActionableError provides mechanism for it.

[Andrew T Wilson (Slow), 2013-05-14 12:34:00]

https://codereview.chromium.org/14655009/diff/1/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/14655009/diff/1/chrome/common/pref_names.cc#n...
chrome/common/pref_names.cc:1801: const char kSyncDisabledByAdmin[] =
"sync.disabled_by_admin";
On 2013/05/13 22:37:09, pavely wrote:
> On 2013/05/13 04:18:58, timsteele wrote:
> > Can eventually replace sync.managed with this?  If so, can you add a TODO
here
> > and reference a bug#?
> We can replace sync.managed once controlling IsManaged flag through group
policy
> on windows is deprecated. I don't know if/when this is planned. Will add TODO.

This won't be deprecated - we still need this so enterprise admins for
non-dasher domains still have a way to disable sync.

https://codereview.chromium.org/14655009/diff/1/chrome/common/pref_names.cc#n...
chrome/common/pref_names.cc:1801: const char kSyncDisabledByAdmin[] =
"sync.disabled_by_admin";
On 2013/05/13 22:37:09, pavely wrote:
> On 2013/05/13 11:51:13, Andrew T Wilson wrote:
> > I don't think this is the right way to approach this - we should not be
> storing
> > permanent client-side state that the user has disabled sync on the server.
> > 
> > This is especially true because on some platforms (ChromeOS, enterprise
> desktop)
> > you can't sign out. A better solution is to keep some flag in memory in PSS
> that
> > says that sync was disabled by the server, which is cleared on restart of
> > Chrome.
> > 
> > I thought that was the approach we were going with from our previous
> discussions
> > - is a permanent pref required for some reason?
> Flag is stored in prefs to display settings page correctly. Currently values
> that affect states of buttons on settings page are kept in prefs and don't
> require sync backend to be initialized and attempted to talk to server.
This is not strictly true - browser_options_handler.cc calls
ProfileSyncService::HasUnrecoverableError() which is not backed by prefs.
Likewise, sync_ui_util::GetStatusLabels() which is also called by
BrowserOptionsHandler queries a bunch of runtime information from
ProfileSyncService (such as the current auth error) which is not backed by
prefs.

I think it's OK if the settings page does not show sync as disabled until after
the sync backend is initialized.

> I tried
> to implement ProfileSyncService to talk to service right after initialization,
> but that would mean issuing RPC every time right after browser startup. I
think
> we shouldn't be doing this and therefore I decided to cache this flag in
prefs.
> I'll work on the change where we will periodically (infrequently) initiallize
> backend to check if account is still disabled.

I don't think I can approve this change with the disabled status stored in the
pref - the correct way to do this is to have PSS check is_initialized(), not to
try to cache the data in prefs so you can access the previous value while your
service is initializing (that's not what prefs is for).

Even if you sporadically refresh the prefs value, this is still the wrong
approach.

https://codereview.chromium.org/14655009/diff/11001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile.cc (right):

https://codereview.chromium.org/14655009/diff/11001/chrome/browser/profiles/p...
chrome/browser/profiles/profile.cc:155: !prefs.IsSyncDisabledByAdmin();
Agreed with Tim that it's too bad that we have so many different ways for sync
to be disabled, each with different connotations. Some consolidation would be
good, but I think that this is something we can pursue in a separate CL.

https://codereview.chromium.org/14655009/diff/11001/chrome/browser/sync/profi...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/14655009/diff/11001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service.cc:1936:
sync_prefs_.SetSyncDisabledByAdmin(false);
As I mentioned previously, on ChromeOS the user never signs out so this code
will never get run on ChromeOS.

[tim (not reviewing), 2013-05-16 18:32:48]

https://codereview.chromium.org/14655009/diff/1/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/14655009/diff/1/chrome/common/pref_names.cc#n...
chrome/common/pref_names.cc:1801: const char kSyncDisabledByAdmin[] =
"sync.disabled_by_admin";
On 2013/05/14 12:34:00, Andrew T Wilson wrote:
> On 2013/05/13 22:37:09, pavely wrote:
> > On 2013/05/13 11:51:13, Andrew T Wilson wrote:
> > > I don't think this is the right way to approach this - we should not be
> > storing
> > > permanent client-side state that the user has disabled sync on the server.
> > > 
> > > This is especially true because on some platforms (ChromeOS, enterprise
> > desktop)
> > > you can't sign out. A better solution is to keep some flag in memory in
PSS
> > that
> > > says that sync was disabled by the server, which is cleared on restart of
> > > Chrome.
> > > 
> > > I thought that was the approach we were going with from our previous
> > discussions
> > > - is a permanent pref required for some reason?
> > Flag is stored in prefs to display settings page correctly. Currently values
> > that affect states of buttons on settings page are kept in prefs and don't
> > require sync backend to be initialized and attempted to talk to server.
> This is not strictly true - browser_options_handler.cc calls
> ProfileSyncService::HasUnrecoverableError() which is not backed by prefs.
> Likewise, sync_ui_util::GetStatusLabels() which is also called by
> BrowserOptionsHandler queries a bunch of runtime information from
> ProfileSyncService (such as the current auth error) which is not backed by
> prefs.
> 
> I think it's OK if the settings page does not show sync as disabled until
after
> the sync backend is initialized.
> 
> > I tried
> > to implement ProfileSyncService to talk to service right after
initialization,
> > but that would mean issuing RPC every time right after browser startup. I
> think
> > we shouldn't be doing this and therefore I decided to cache this flag in
> prefs.
> > I'll work on the change where we will periodically (infrequently)
initiallize
> > backend to check if account is still disabled.
> 
> I don't think I can approve this change with the disabled status stored in the
> pref - the correct way to do this is to have PSS check is_initialized(), not
to
> try to cache the data in prefs so you can access the previous value while your
> service is initializing (that's not what prefs is for).
> 
> Even if you sporadically refresh the prefs value, this is still the wrong
> approach.

Maybe I'm missing something. We store a bit in prefs to tell us whether sync
setup has completed.  How are you proposing we know, when we restart Chrome,
that the reason this bit is false / sync is disabled is because of policy? 

Are you proposing we leave all the sync machinery up and running in a permanent
lame-duck state, where on restart we wait for the user to do something that will
trigger sync initialization (could take a while, especially with my deferred
init work) so that we re-learn the value of this single bit?  That seems like
considerable overkill  / complexity (vs "if enterprise disables sync, disable it
on the client"). What's the motivation for doing that?

[pavely, 2013-05-21 01:21:57]

> I don't think I can approve this change with the disabled status stored in the
> pref - the correct way to do this is to have PSS check is_initialized(), not
to
> try to cache the data in prefs so you can access the previous value while your
> service is initializing (that's not what prefs is for).
> 
> Even if you sporadically refresh the prefs value, this is still the wrong
> approach.

Andrew,

Here are the reasons why I decided to store disabled state in prefs, not only in
PSS state:

In typical scenario sync is disabled for account at the time of signin, meaning
HasSyncSetupCompleted() will always be false. With this
ProfileSyncService::TryStart will only start backend on desktop if user clicks
setup button. ProfileSyncService will never try to contact server and will never
discover that account was reenabled.

At the same time when user navigates to settings page it will not reflect
correct state for sync buttons, when user clicks on "Setup sync"
ProfileSyncService will discover that account is disabled and update UI which
IMO will cause inconsistent UX.

Even if PSS finds out that account was reenabled, it cannot start syncing right
away, it has to show sync settings UI to user. Showing this UI cannot be
automatic, it needs to be explicitly initiated by user. I think chrome needs to
show badge first and then show correct explanation on settings page that account
was reenabled with corresponding buttons.

Also it matches behavior of IsManaged policy setting:
SyncPolicyHandler::ApplyPolicySettings only disables sync if IsManaged== true,
but not reenables if it is false.

What do you think?

[Andrew T Wilson (Slow), 2013-05-21 09:40:50]

On 2013/05/16 18:32:48, timsteele wrote:
> 
> Are you proposing we leave all the sync machinery up and running in a
permanent
> lame-duck state, where on restart we wait for the user to do something that
will
> trigger sync initialization (could take a while, especially with my deferred
> init work) so that we re-learn the value of this single bit?  That seems like
> considerable overkill  / complexity (vs "if enterprise disables sync, disable
it
> on the client"). What's the motivation for doing that?

What I don't want to have happen is: the enterprise who is administering a
domain of 30000 users accidentally turns off sync via the dasher panel. A few
hours later he realizes his mistake and turns it back on, but at this point sync
has been permanently disabled on the client across every single user of his
domain, and they have to delete their profiles (can't sign out if they have
policy) and sign back in.

I guess it just isn't clear to me exactly what UX flow issues this pref was
supposed to resolve. It sounds like there are two cases:

1) Sync is disabled during user signin.
2) Sync is disabled *after* the user signs in.

In case #2, HasSyncSetupCompleted() == true, so we'll always know that sync is
disabled when we contact the sync server during startup, so we can display
appropriate UI on the settings page based on dynamic state in PSS (no pref
required). The key thing here is that if the admin re-enables sync, sync will
just start up, so we avoid the "oops, I just accidentally permanently disabled
sync on my entire domain" issue.

In case #1, HasSyncSetupCompleted() == false (on desktop - on ChromeOS it's
always true so we always fall into case #2 above and automatically retry sync on
every startup). I think this already correctly displays the
SIGNED_IN_WITH_SYNC_SUPPRESSED UI after Raghu's recent checkin
(https://codereview.chromium.org/14691004/diff/202002/chrome/browser/sync/sync...)

So, I guess what I'm saying is that after Raghu's CL, I think the UX is fine and
we don't need this kind of "sync is permanently disabled" flag. If, for some
reason, we decide that we *do* want to permanently disable sync just because the
admin has temporarily turned off sync on the server, then there are already
methods in place to achieve that (IsStartSuppressed()) and we don't need to add
more.

Happy to chat more about this over VC if that would help bring this to a more
speedy conclusion.

[pavely, 2013-05-23 01:12:26]

Andrew, could you take a look?

This is implementation with "sync disabled" flag kept in memory and reset after
restart.

[Andrew T Wilson (Slow), 2013-05-24 15:13:10]

LGTM if we can make the change I suggest to IsSyncAccessible()

https://codereview.chromium.org/14655009/diff/26001/chrome/browser/signin/sig...
File chrome/browser/signin/signin_tracker.cc (right):

https://codereview.chromium.org/14655009/diff/26001/chrome/browser/signin/sig...
chrome/browser/signin/signin_tracker.cc:176: if (service->IsManaged())
Rather than making every caller of Profile::IsSyncAccessible() also call
service->IsManaged(), it seems cleaner to change IsSyncAccessible() to do this:

bool Profile::IsSyncAccessible() {
  if (ProfileSyncServiceFactory::HasProfileSyncService(this))
    return !ProfileSyncServiceFactory::GetProfileSyncService(this)->IsManaged();

  // No ProfileSyncService created yet - we don't want to create one, so just
infer the accessible state
  // by looking at prefs/command line flags.
  browser_sync::SyncPrefs prefs(GetPrefs());
  return ProfileSyncService::IsSyncEnabled() && !prefs.IsManaged())
}

https://codereview.chromium.org/14655009/diff/26001/chrome/browser/signin/sig...
chrome/browser/signin/signin_tracker.cc:202: if (service->IsManaged())
See my previous comment - if IsSyncAccessible() calls service->IsManaged() then
we don't have to explicitly call it here.

https://codereview.chromium.org/14655009/diff/26001/chrome/browser/sync/profi...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/14655009/diff/26001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service.cc:1226: // restart
nit: sentence punctuation (add a period at the end)

https://codereview.chromium.org/14655009/diff/26001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service.cc:2022: return sync_prefs_.IsManaged()
|| sync_disabled_by_admin_;
I'm OK with this change even though it means we now diverge from
SyncPrefs::IsManaged(), but we should change Profile::IsSyncAccessible() to call
this if possible.

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

https://codereview.chromium.org/14655009/diff/26001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service.h:848: // ProfileSyncService from
starting backend till browser restart.
Technically, I think PSS should try to start the backend if the user signed out
and signed in again because they might be signing in with a different account
the second time. Perhaps we can make sure this is cleared when the user signs
out?

https://codereview.chromium.org/14655009/diff/26001/chrome/browser/ui/sync/on...
File chrome/browser/ui/sync/one_click_signin_sync_starter.cc (right):

https://codereview.chromium.org/14655009/diff/26001/chrome/browser/ui/sync/on...
chrome/browser/ui/sync/one_click_signin_sync_starter.cc:302: if
(!profile_sync_service->IsManaged()) {
If you move IsManaged() into IsSyncAccessible(), then GetProfileSyncService()
will return NULL and you won't need these explicit calls.

[pavely, 2013-05-24 22:17:10]

Andrew, your advice didn't work out as easily. Could you take a look at my
comments?

https://codereview.chromium.org/14655009/diff/26001/chrome/browser/signin/sig...
File chrome/browser/signin/signin_tracker.cc (right):

https://codereview.chromium.org/14655009/diff/26001/chrome/browser/signin/sig...
chrome/browser/signin/signin_tracker.cc:176: if (service->IsManaged())
On 2013/05/24 15:13:10, Andrew T Wilson wrote:
> Rather than making every caller of Profile::IsSyncAccessible() also call
> service->IsManaged(), it seems cleaner to change IsSyncAccessible() to do
this:
> 
> bool Profile::IsSyncAccessible() {
>   if (ProfileSyncServiceFactory::HasProfileSyncService(this))
>     return
!ProfileSyncServiceFactory::GetProfileSyncService(this)->IsManaged();
> 
>   // No ProfileSyncService created yet - we don't want to create one, so just
> infer the accessible state
>   // by looking at prefs/command line flags.
>   browser_sync::SyncPrefs prefs(GetPrefs());
>   return ProfileSyncService::IsSyncEnabled() && !prefs.IsManaged())
> }

I tried this in my experiments. The issue is that ProfileSyncService transitions

from accessible to not-accessible state while setup is in progress. 
OneClickSigninSyncStarter calls SetSetupInProgress(true) in the beginning, it 
should call SetSetupInProgress(false) at the end, otherwise after user signs out
settings page will disable button and show that setup is in progress. If
GetProfileSyncService returns NULL for disabled user then 
OneClickSigninSyncStarter won't have PSS pointer to call 
SetSetupInProgress(false). This reminds me that I'm not handling case of user
selecting "Choose what to sync" correctly.
Another solution for this problem is to call SetSetupInProgress(false) when 
I set disabled flag in PSS, but I think it hides dependency in implementation 
that shouldn't be there.

https://codereview.chromium.org/14655009/diff/26001/chrome/browser/sync/profi...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/14655009/diff/26001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service.cc:1226: // restart
On 2013/05/24 15:13:10, Andrew T Wilson wrote:
> nit: sentence punctuation (add a period at the end)

Done.

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

https://codereview.chromium.org/14655009/diff/26001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service.h:848: // ProfileSyncService from
starting backend till browser restart.
On 2013/05/24 15:13:10, Andrew T Wilson wrote:
> Technically, I think PSS should try to start the backend if the user signed
out
> and signed in again because they might be signing in with a different account
> the second time. Perhaps we can make sure this is cleared when the user signs
> out?

Done.

https://codereview.chromium.org/14655009/diff/26001/chrome/browser/ui/sync/on...
File chrome/browser/ui/sync/one_click_signin_sync_starter.cc (right):

https://codereview.chromium.org/14655009/diff/26001/chrome/browser/ui/sync/on...
chrome/browser/ui/sync/one_click_signin_sync_starter.cc:302: if
(!profile_sync_service->IsManaged()) {
On 2013/05/24 15:13:10, Andrew T Wilson wrote:
> If you move IsManaged() into IsSyncAccessible(), then GetProfileSyncService()
> will return NULL and you won't need these explicit calls.

I need pointer to PSS to call SetSetupInProgress(false). See my
comment in signin_tracker.cc

[Andrew T Wilson (Slow), 2013-05-27 15:03:33]

Still LGTM.

On second thought, I'm OK with not moving IsManaged() into IsSyncAccessible().
Can you put a comment in IsSyncAccessible() about why we don't call it (because
it can change on the fly during sync setup)?

https://codereview.chromium.org/14655009/diff/26001/chrome/browser/signin/sig...
File chrome/browser/signin/signin_tracker.cc (right):

https://codereview.chromium.org/14655009/diff/26001/chrome/browser/signin/sig...
chrome/browser/signin/signin_tracker.cc:176: if (service->IsManaged())
On 2013/05/24 22:17:11, pavely wrote:
> On 2013/05/24 15:13:10, Andrew T Wilson wrote:
> > Rather than making every caller of Profile::IsSyncAccessible() also call
> > service->IsManaged(), it seems cleaner to change IsSyncAccessible() to do
> this:
> > 
> > bool Profile::IsSyncAccessible() {
> >   if (ProfileSyncServiceFactory::HasProfileSyncService(this))
> >     return
> !ProfileSyncServiceFactory::GetProfileSyncService(this)->IsManaged();
> > 
> >   // No ProfileSyncService created yet - we don't want to create one, so
just
> > infer the accessible state
> >   // by looking at prefs/command line flags.
> >   browser_sync::SyncPrefs prefs(GetPrefs());
> >   return ProfileSyncService::IsSyncEnabled() && !prefs.IsManaged())
> > }
> 
> I tried this in my experiments. The issue is that ProfileSyncService
transitions
> 
> from accessible to not-accessible state while setup is in progress. 
> OneClickSigninSyncStarter calls SetSetupInProgress(true) in the beginning, it 
> should call SetSetupInProgress(false) at the end, otherwise after user signs
out
> settings page will disable button and show that setup is in progress. If
> GetProfileSyncService returns NULL for disabled user then 
> OneClickSigninSyncStarter won't have PSS pointer to call 
> SetSetupInProgress(false). This reminds me that I'm not handling case of user
> selecting "Choose what to sync" correctly.
> Another solution for this problem is to call SetSetupInProgress(false) when 
> I set disabled flag in PSS, but I think it hides dependency in implementation 
> that shouldn't be there.

Yeah, that's a funky edge case. You could deal with it by having the code that
calls SetSetupInProgress(false) call ProfileSyncServiceFactory::GetForProfile()
to grab the PSS instead of calling IsProfileAccessible() in that specific case.
It could still return null if sync is disabled via a command-line flag so we
have to check for null.

I think it's better to explicitly handle the case of sync changing its
accessibility than to make everyone manually check the disabled status
separately. I just know from hard experience that if you don't do this, people
will forget to do it in their code and we'll get breakages when people disable
sync on the domain (in the same way that sporadically we get crashes when you
run chrome with --sync-disabled because people forget to check for a null PSS).

[commit-bot: I haz the power, 2013-05-29 00:40:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pavely@chromium.org/14655009/43002

[pavely, 2013-05-29 00:41:13]

I've updated CL. Thanks.

[commit-bot: I haz the power, 2013-05-29 01:00:39]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-05-29 07:00:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pavely@chromium.org/14655009/43002

[commit-bot: I haz the power, 2013-05-29 07:09:21]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[pavely, 2013-05-29 17:13:09]

+rlp & erg for profile.cc
Rachel, Elliot, could one of you take a look at profile.cc?

[Elliot Glaysher, 2013-05-29 18:05:31]

lgtm

[commit-bot: I haz the power, 2013-05-29 18:14:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pavely@chromium.org/14655009/43002

[commit-bot: I haz the power, 2013-05-29 20:45:00]

Change committed as 202956