[Sync] Fix sync backup for supervised users

[Sync] Fix sync backup for supervised users

sync_initialized() does not account for backup/rollback modes. For now we
just manually check the backend mode. In the future the PSS will expose
a better method for denoting when its safe to reconfigure.

BUG=417400
Committed: https://crrev.com/5abf3d89f8c8416fd48635470e4f87ddad19f5ef
Cr-Commit-Position: refs/heads/master@{#296968}

[Nicolas Zea, 2014-09-24 19:56:18]

zea@chromium.org changed reviewers:
+ treib@chromium.org

[Nicolas Zea, 2014-09-24 19:56:19]

+Marc, PTAL. I'd like to add an integration test as well, but I'm not sure what
the best way to make a profile be supervised in a test. Do you know if there are
any existing sync integration tests for supervised users?

[Nicolas Zea, 2014-09-24 21:22:45]

+Patrick FYI

[Marc Treib, 2014-09-25 09:31:51]

I don't think we have any integration tests for Sync + SUs yet. +bauerb who
might know more.

On making a profile supervised: One way to do it (in a browser test) is to
specify a supervised user id via the command line,
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/sup...

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

https://codereview.chromium.org/605483002/diff/1/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.cc:333: need_backup_ =
(signin_->GetEffectiveUsername().empty() ||
If you want to exclude supervised users, I think it would be easier to not use
the SigninManagerWrapper here, and instead just ask the SigninManager directly
for the username, i.e.
signin_->GetOriginal()->GetAuthenticatedUsername().empty()
That way you don't need the extra IsSupervised check.

[Bernhard Bauer, 2014-09-25 16:20:01]

On 2014/09/25 09:31:51, Marc Treib wrote:
> I don't think we have any integration tests for Sync + SUs yet. +bauerb who
> might know more.

There is a rather basic smoke test at
chrome/browser/sync/test/integration/single_client_supervised_user_settings_sync_test.cc.

[Bernhard Bauer, 2014-09-25 16:20:03]

On 2014/09/25 09:31:51, Marc Treib wrote:
> I don't think we have any integration tests for Sync + SUs yet. +bauerb who
> might know more.

There is a rather basic smoke test at
chrome/browser/sync/test/integration/single_client_supervised_user_settings_sync_test.cc.

[Nicolas Zea, 2014-09-26 01:25:02]

Patchset #2 (id:20001) has been deleted

[Nicolas Zea, 2014-09-26 01:26:48]

PTAL.

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

https://codereview.chromium.org/605483002/diff/1/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.cc:333: need_backup_ =
(signin_->GetEffectiveUsername().empty() ||
On 2014/09/25 09:31:51, Marc Treib wrote:
> If you want to exclude supervised users, I think it would be easier to not use
> the SigninManagerWrapper here, and instead just ask the SigninManager directly
> for the username, i.e.
> signin_->GetOriginal()->GetAuthenticatedUsername().empty()
> That way you don't need the extra IsSupervised check.

Unfortunately I don't think that will work. The first sync time is still null. I
need to distinguish that its a supervised user and have that override the other
logic.

That said, the alternative is to fix the logic in supervised user service and
make backup work properly. The new patchset does that (although the integration
tests need more work to have proper coverage. Filed 417930 for that).

[Marc Treib, 2014-09-26 08:42:38]

I think fixing the SUService is definitely the right thing to do (whether or not
we also disable sync backup for SUs).

https://codereview.chromium.org/605483002/diff/40001/chrome/browser/supervise...
File chrome/browser/supervised_user/supervised_user_service.cc (right):

https://codereview.chromium.org/605483002/diff/40001/chrome/browser/supervise...
chrome/browser/supervised_user/supervised_user_service.cc:407: void
SupervisedUserService::OnStateChanged() {
This does get called when the backend_mode changes, right?
If so, can you please update the comment at
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/syn...
?

https://codereview.chromium.org/605483002/diff/40001/chrome/browser/supervise...
chrome/browser/supervised_user/supervised_user_service.cc:454:
DCHECK(service->sync_initialized() &&
Hm, so the underlying problem was that sync_initialized() alone doesn't any
longer mean that we may call OnUserChoseDatatypes/SetSyncSetupCompleted? Maybe
this should be exposed via some "bool PSS::IsReadyForAction()", and mentioned in
the comment for sync_initialized? It seems like more users of the PSS might trip
over that.

[Marc Treib, 2014-09-26 08:45:01]

treib@chromium.org changed reviewers:
+ bauerb@chromium.org

[Marc Treib, 2014-09-26 08:45:01]

Please update the description before committing!

Also, +bauerb since I'm not an OWNER yet.

[Bernhard Bauer, 2014-09-26 09:20:39]

lgtm

[Nicolas Zea, 2014-09-26 17:09:22]

https://codereview.chromium.org/605483002/diff/40001/chrome/browser/supervise...
File chrome/browser/supervised_user/supervised_user_service.cc (right):

https://codereview.chromium.org/605483002/diff/40001/chrome/browser/supervise...
chrome/browser/supervised_user/supervised_user_service.cc:407: void
SupervisedUserService::OnStateChanged() {
On 2014/09/26 08:42:38, Marc Treib wrote:
> This does get called when the backend_mode changes, right?
> If so, can you please update the comment at
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/syn...
> ?

It does. And it looks like that comment is quite out of date. I'd like to do an
audit of all the NotifyObservers callsites separately, and update the comment
based on that. Filed crbug.com/418140 for that.

https://codereview.chromium.org/605483002/diff/40001/chrome/browser/supervise...
chrome/browser/supervised_user/supervised_user_service.cc:454:
DCHECK(service->sync_initialized() &&
On 2014/09/26 08:42:38, Marc Treib wrote:
> Hm, so the underlying problem was that sync_initialized() alone doesn't any
> longer mean that we may call OnUserChoseDatatypes/SetSyncSetupCompleted? Maybe
> this should be exposed via some "bool PSS::IsReadyForAction()", and mentioned
in
> the comment for sync_initialized? It seems like more users of the PSS might
trip
> over that.

Yeah, I agree this is confusing. For now, since this patch needs to be merged,
I'd like to minimize the changes in other areas. Filed crbug.com/418141

[Nicolas Zea, 2014-09-26 17:09:32]

The CQ bit was checked by zea@chromium.org

[commit-bot: I haz the power, 2014-09-26 17:10:01]

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

[Nicolas Zea, 2014-09-26 17:13:30]

The CQ bit was unchecked by zea@chromium.org

[Nicolas Zea, 2014-09-26 17:16:15]

The CQ bit was checked by zea@chromium.org

[commit-bot: I haz the power, 2014-09-26 17:17:09]

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

[commit-bot: I haz the power, 2014-09-26 17:24:59]

Committed patchset #2 (id:40001) as 7457d4c868b0c6db9f60f35848ea475336185315

[commit-bot: I haz the power, 2014-09-26 17:25:46]

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