Disable sync for Lightspeed experimentation.

Disable sync for Lightspeed experimentation.

We want to determine Sync's contribution to Browser startup. As such,
we will add code allowing us to disable Sync (or a part thereof) via
Finch.

The SupervisedUserService was/is not robust to disabling Sync (it
crashes). Since disabling sync is a generally supported feature
(there's a command line flag), that is now fixed.

BUG=454788
Committed: https://crrev.com/4cda8457e67bfd338eb4cd82b94c48084f15685f
Cr-Commit-Position: refs/heads/master@{#317642}

[Mike Lerman, 2015-02-18 18:58:01]

mlerman@chromium.org changed reviewers:
+ asvitkine@chromium.org, bauerb@chromium.org, zea@chromium.org

[Mike Lerman, 2015-02-18 18:58:02]

Hi,

Please review the attached CL. Thanks!

zea - c/b/sync, make sure I'm disabling sync properly
bauerb - c/b/supervised_user, the changes we discussed about the dependency on
the ProfileSyncService (which can be null)
asvitkine - c/b/sync's the calls to variations::, to make sure I'm doing that
properly.

Thanks,
Mike

[Bernhard Bauer, 2015-02-18 21:12:44]

Oh, uh, is there a way we can not do that for supervised users? It's true that
disabling Sync via the command line is supported in principle (in the sense that
we shouldn't crash with the flag), but the whole experience for supervised users
relies on Sync and is going to be significantly broken without it.

(The reason why don't care about the command line here is because it's outside
of the threat model: It's possible to lock down the OS so that Chrome can only
be started with a fixed command line, which makes it impossible to disable Sync
for supervised users if the OS is set up that way. That doesn't apply for
randomly-assigned Finch trials.)

(Also, as an aside: Isn't this basically going to degrade the experience for
users who want to use Sync and have previously been using it happily?)

Is there a launch bug for this or should we file one? I feel it might be a good
idea to loop in product owners (for example for supervised users, but I could
also imagine other owners would like a heads-up).

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

https://codereview.chromium.org/933293005/diff/1/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.cc:2462: return false;
Nit: indent two spaces less.

[Mike Lerman, 2015-02-18 21:23:49]

rpop@, the Sync PM, is aware of this work, but I can always CC her.

I don't mind exempting supervised user-profiles from this disablement. They
won't significant affect the results.

This will degrade the sync-er's experience for sure. What we're likely to do is
launch this to 5% of canary for 2 days and collect results, so hopefully it
won't be a larger impact than if Sync was just broken in a day's canary.

There's no launch bug for this - this is one of many experiments being done
under the Chrome Fast umbrella. Nothing about this will ever go past Canary.

[Bernhard Bauer, 2015-02-18 22:56:57]

True, on Canary, users are probably more resistant to breakage... I would
probably still exempt supervised users though.


 +nepper, FYI


On Wed, Feb 18, 2015, 21:23 null <mlerman@chromium.org> wrote:

rpop@, the Sync PM, is aware of this work, but I can always CC her.

I don't mind exempting supervised user-profiles from this disablement. They
won't significant affect the results.

This will degrade the sync-er's experience for sure. What we're likely to
do is
launch this to 5% of canary for 2 days and collect results, so hopefully it
won't be a larger impact than if Sync was just broken in a day's canary.

There's no launch bug for this - this is one of many experiments being done
under the Chrome Fast umbrella. Nothing about this will ever go past Canary.

https://codereview.chromium.org/933293005/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Nicolas Zea, 2015-02-19 21:26:29]

sync LGTM.

[Alexei Svitkine (slow), 2015-02-19 22:04:08]

lgtm % mine and others' comments

https://codereview.chromium.org/933293005/diff/1/chrome/browser/supervised_us...
File chrome/browser/supervised_user/supervised_user_service.cc (right):

https://codereview.chromium.org/933293005/diff/1/chrome/browser/supervised_us...
chrome/browser/supervised_user/supervised_user_service.cc:644:
profile_sync_service_ =
Initialize this in the ctor too, or some bots will yell at you.

https://codereview.chromium.org/933293005/diff/1/chrome/browser/supervised_us...
File chrome/browser/supervised_user/supervised_user_service.h (right):

https://codereview.chromium.org/933293005/diff/1/chrome/browser/supervised_us...
chrome/browser/supervised_user/supervised_user_service.h:338:
ProfileSyncService* profile_sync_service_;  // owned by BCKS factory.
Nit: If you already have a comment above, merge this comment into it. Also, use
full sentences.

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

https://codereview.chromium.org/933293005/diff/1/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.cc:2460: // TODO(mlerman): Remove this
after the experiment. crbug.com/454788
Nit: Add this comment above the other variations check too.

[Mike Lerman, 2015-02-20 16:36:21]

New patchsets have been uploaded after l-g-t-m from
zea@chromium.org,asvitkine@chromium.org

[Mike Lerman, 2015-02-20 16:38:02]

Okay - this CL now exempts Supervised Users from having Sync turned off. This
required me to move the check out of ProfileSyncService::IsSyncEnabled, since
that method's static.

zea@, take a second look if you could, please :)

Thanks!

https://codereview.chromium.org/933293005/diff/1/chrome/browser/supervised_us...
File chrome/browser/supervised_user/supervised_user_service.cc (right):

https://codereview.chromium.org/933293005/diff/1/chrome/browser/supervised_us...
chrome/browser/supervised_user/supervised_user_service.cc:644:
profile_sync_service_ =
On 2015/02/19 22:04:08, Alexei Svitkine wrote:
> Initialize this in the ctor too, or some bots will yell at you.

Done.

https://codereview.chromium.org/933293005/diff/1/chrome/browser/supervised_us...
File chrome/browser/supervised_user/supervised_user_service.h (right):

https://codereview.chromium.org/933293005/diff/1/chrome/browser/supervised_us...
chrome/browser/supervised_user/supervised_user_service.h:338:
ProfileSyncService* profile_sync_service_;  // owned by BCKS factory.
On 2015/02/19 22:04:08, Alexei Svitkine wrote:
> Nit: If you already have a comment above, merge this comment into it. Also,
use
> full sentences.

It is my humblest pleasure to use the fullest and most complete of sentences. Ok
GR8 CU L8R.

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

https://codereview.chromium.org/933293005/diff/1/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.cc:2460: // TODO(mlerman): Remove this
after the experiment. crbug.com/454788
On 2015/02/19 22:04:08, Alexei Svitkine wrote:
> Nit: Add this comment above the other variations check too.

Done.

https://codereview.chromium.org/933293005/diff/1/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.cc:2462: return false;
On 2015/02/18 21:12:44, Bernhard Bauer wrote:
> Nit: indent two spaces less.

Done.

[Bernhard Bauer, 2015-02-20 16:45:27]

Thanks, LGTM!

[Nicolas Zea, 2015-02-20 18:23:26]

still lgtm with a nit

https://codereview.chromium.org/933293005/diff/20001/chrome/browser/sync/prof...
File chrome/browser/sync/profile_sync_service_factory.cc (right):

https://codereview.chromium.org/933293005/diff/20001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service_factory.cc:58:
!variations::GetVariationParamValue("LightSpeed", "DisableSync").empty())
nit: curly braces

[Mike Lerman, 2015-02-20 18:28:10]

New patchsets have been uploaded after l-g-t-m from
bauerb@chromium.org,zea@chromium.org

[Mike Lerman, 2015-02-20 18:28:50]

https://codereview.chromium.org/933293005/diff/20001/chrome/browser/sync/prof...
File chrome/browser/sync/profile_sync_service_factory.cc (right):

https://codereview.chromium.org/933293005/diff/20001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service_factory.cc:58:
!variations::GetVariationParamValue("LightSpeed", "DisableSync").empty())
On 2015/02/20 18:23:26, Nicolas Zea wrote:
> nit: curly braces

Done. The line wrapping's a bit ugly so I let git cl format do it.

[Mike Lerman, 2015-02-23 18:59:04]

The CQ bit was checked by mlerman@chromium.org

[commit-bot: I haz the power, 2015-02-23 19:00:06]

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

[commit-bot: I haz the power, 2015-02-23 20:34:24]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-02-23 20:34:51]

Patchset 4 (id:??) landed as
https://crrev.com/4cda8457e67bfd338eb4cd82b94c48084f15685f
Cr-Commit-Position: refs/heads/master@{#317642}