[Sync] Fix EnableDisableSingleClientTest

[Sync] Fix EnableDisableSingleClientTest

BUG=689662
Review-Url: https://codereview.chromium.org/2740363002
Cr-Commit-Position: refs/heads/master@{#457959}
Committed: https://chromium.googlesource.com/chromium/src/+/3f8c8d163c769d9b890e8eeb29d1ea1001e312ad

[Gang Wu, 2017-03-10 18:26:38]

The CQ bit was checked by gangwu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-10 18:27:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-10 19:16:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-10 19:16:32]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Gang Wu, 2017-03-14 16:50:55]

Description was changed from

==========
try fix

BUG=
==========

to

==========
try fix

BUG=
==========

[Gang Wu, 2017-03-14 16:51:45]

Description was changed from

==========
try fix

BUG=
==========

to

==========
[Sync] Add switch for ARC_PACKAGE

After add switch, two of sync_integration_tests could be enabled.

BUG=689662
==========

[Gang Wu, 2017-03-14 16:53:59]

gangwu@chromium.org changed reviewers:
+ lgcheng@google.com

[Gang Wu, 2017-03-14 16:54:08]

Patchset #1 (id:1) has been deleted

[Gang Wu, 2017-03-14 16:54:13]

The CQ bit was checked by gangwu@chromium.org to run a CQ dry run

[Gang Wu, 2017-03-14 16:54:25]

Patchset #1 (id:20001) has been deleted

[commit-bot: I haz the power, 2017-03-14 16:54:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-14 17:33:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-14 17:33:18]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Gang Wu, 2017-03-14 20:03:40]

The CQ bit was checked by gangwu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-14 20:04:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-14 21:26:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-14 21:26:20]

Dry run: This issue passed the CQ dry run.

[Gang Wu, 2017-03-14 21:34:09]

PTAL

[skym, 2017-03-14 21:39:05]

skym@chromium.org changed reviewers:
+ skym@chromium.org

[skym, 2017-03-14 21:39:06]

https://codereview.chromium.org/2740363002/diff/60001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/enable_disable_test.cc (right):

https://codereview.chromium.org/2740363002/diff/60001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/enable_disable_test.cc:161:
IN_PROC_BROWSER_TEST_F(EnableDisableSingleClientTest, EnableOneAtATime) {
Why are these tests being fixed by this change? Am I correct to assume that
arc::IsArcAllowedForProfile() returns false for these two tests? Wouldn't we
prefer these tests to include ARC_PACKAGE? Is there some setup call we need to
make to make these work instead?

[lgcheng, 2017-03-14 21:59:28]

https://codereview.chromium.org/2740363002/diff/60001/chrome/browser/sync/chr...
File chrome/browser/sync/chrome_sync_client.cc (right):

https://codereview.chromium.org/2740363002/diff/60001/chrome/browser/sync/chr...
chrome/browser/sync/chrome_sync_client.cc:683: if
(arc::IsArcAllowedForProfile(profile_)) {
I am not sure about the test infrastructure.
But IMO the switch is a little redundant. The todo here is because at first
deployment we use default datatypecontroller and had no control of
ArcPackageSyncService, start or stop.

In later update, we overload readyForStart() method in the data controller. If
arc is not allowed the controller will be marked as not ready to start. And I
should remove the TODO here at that moment.

Also, arc::IsArcAllowedForProfile() returns false for the test profile(s), and
we only let readyForStart() to return true for sync integration test. So maybe
just remove the TODO is better?

[Gang Wu, 2017-03-15 07:55:46]

reply comments

https://codereview.chromium.org/2740363002/diff/60001/chrome/browser/sync/chr...
File chrome/browser/sync/chrome_sync_client.cc (right):

https://codereview.chromium.org/2740363002/diff/60001/chrome/browser/sync/chr...
chrome/browser/sync/chrome_sync_client.cc:683: if
(arc::IsArcAllowedForProfile(profile_)) {
On 2017/03/14 21:59:28, lgcheng wrote:
> I am not sure about the test infrastructure.
> But IMO the switch is a little redundant. The todo here is because at first
> deployment we use default datatypecontroller and had no control of
> ArcPackageSyncService, start or stop.
> 
> In later update, we overload readyForStart() method in the data controller. If
> arc is not allowed the controller will be marked as not ready to start. And I
> should remove the TODO here at that moment.
> 
> Also, arc::IsArcAllowedForProfile() returns false for the test profile(s), and
> we only let readyForStart() to return true for sync integration test. So maybe
> just remove the TODO is better?
> 

Add a switch here not just for fixing test, but also for saving memory.
Normally, we only create controller when necessary.
If the data type is not enabled, the controller won't be created. I think most
of controllers did this way.
Of course, if you do have reason to always create arc controller in chromeos, I
am fine with that, I can fix test in other way.

https://codereview.chromium.org/2740363002/diff/60001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/enable_disable_test.cc (right):

https://codereview.chromium.org/2740363002/diff/60001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/enable_disable_test.cc:161:
IN_PROC_BROWSER_TEST_F(EnableDisableSingleClientTest, EnableOneAtATime) {
On 2017/03/14 21:39:06, skym wrote:
> Why are these tests being fixed by this change? Am I correct to assume that
> arc::IsArcAllowedForProfile() returns false for these two tests? Wouldn't we
> prefer these tests to include ARC_PACKAGE? Is there some setup call we need to
> make to make these work instead?

Before this change, sync client always create ARC controller whenever ARC is
enabled or not. SyncService has the controller, but client cannot enable the ARC
service.
After this change, client will only create controller when service can be
enabled.

[skym, 2017-03-15 15:45:58]

https://codereview.chromium.org/2740363002/diff/60001/chrome/browser/sync/chr...
File chrome/browser/sync/chrome_sync_client.cc (right):

https://codereview.chromium.org/2740363002/diff/60001/chrome/browser/sync/chr...
chrome/browser/sync/chrome_sync_client.cc:683: if
(arc::IsArcAllowedForProfile(profile_)) {
On 2017/03/15 07:55:45, Gang Wu wrote:
> On 2017/03/14 21:59:28, lgcheng wrote:
> > I am not sure about the test infrastructure.
> > But IMO the switch is a little redundant. The todo here is because at first
> > deployment we use default datatypecontroller and had no control of
> > ArcPackageSyncService, start or stop.
> > 
> > In later update, we overload readyForStart() method in the data controller.
If
> > arc is not allowed the controller will be marked as not ready to start. And
I
> > should remove the TODO here at that moment.
> > 
> > Also, arc::IsArcAllowedForProfile() returns false for the test profile(s),
and
> > we only let readyForStart() to return true for sync integration test. So
maybe
> > just remove the TODO is better?
> > 
> 
> Add a switch here not just for fixing test, but also for saving memory.
> Normally, we only create controller when necessary.
> If the data type is not enabled, the controller won't be created. I think most
> of controllers did this way.
> Of course, if you do have reason to always create arc controller in chromeos,
I
> am fine with that, I can fix test in other way. 

I think the piece of information you need to decide if we should gate based on
this flag, is can it change during runtime? If we know at this point, that this
profile is never going to be allowed to use ARC, we don't want to
create/register the controller.

On the other hand, if this can change somehow dynamically, maybe by the user
changing a setting, then we have to still register here, since we don't know.

https://codereview.chromium.org/2740363002/diff/60001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/enable_disable_test.cc (right):

https://codereview.chromium.org/2740363002/diff/60001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/enable_disable_test.cc:161:
IN_PROC_BROWSER_TEST_F(EnableDisableSingleClientTest, EnableOneAtATime) {
On 2017/03/15 07:55:45, Gang Wu wrote:
> On 2017/03/14 21:39:06, skym wrote:
> > Why are these tests being fixed by this change? Am I correct to assume that
> > arc::IsArcAllowedForProfile() returns false for these two tests? Wouldn't we
> > prefer these tests to include ARC_PACKAGE? Is there some setup call we need
to
> > make to make these work instead?
> 
> Before this change, sync client always create ARC controller whenever ARC is
> enabled or not. SyncService has the controller, but client cannot enable the
ARC
> service.
> After this change, client will only create controller when service can be
> enabled.
> 

What I was trying to say is that you're fixing these tests by not registering
the controller. I'd prefer you to fix them by registering the controller and
enabling the ARC machinery.

I have nothing against the selective registration of the ARC controller, but I
want these tests to register it.

[lgcheng, 2017-03-15 17:20:58]

https://codereview.chromium.org/2740363002/diff/60001/chrome/browser/sync/chr...
File chrome/browser/sync/chrome_sync_client.cc (right):

https://codereview.chromium.org/2740363002/diff/60001/chrome/browser/sync/chr...
chrome/browser/sync/chrome_sync_client.cc:683: if
(arc::IsArcAllowedForProfile(profile_)) {
On 2017/03/15 15:45:58, skym wrote:
> On 2017/03/15 07:55:45, Gang Wu wrote:
> > On 2017/03/14 21:59:28, lgcheng wrote:
> > > I am not sure about the test infrastructure.
> > > But IMO the switch is a little redundant. The todo here is because at
first
> > > deployment we use default datatypecontroller and had no control of
> > > ArcPackageSyncService, start or stop.
> > > 
> > > In later update, we overload readyForStart() method in the data
controller.
> If
> > > arc is not allowed the controller will be marked as not ready to start.
And
> I
> > > should remove the TODO here at that moment.
> > > 
> > > Also, arc::IsArcAllowedForProfile() returns false for the test profile(s),
> and
> > > we only let readyForStart() to return true for sync integration test. So
> maybe
> > > just remove the TODO is better?
> > > 
> > 
> > Add a switch here not just for fixing test, but also for saving memory.
> > Normally, we only create controller when necessary.
> > If the data type is not enabled, the controller won't be created. I think
most
> > of controllers did this way.
> > Of course, if you do have reason to always create arc controller in
chromeos,
> I
> > am fine with that, I can fix test in other way. 
> 
> I think the piece of information you need to decide if we should gate based on
> this flag, is can it change during runtime? If we know at this point, that
this
> profile is never going to be allowed to use ARC, we don't want to
> create/register the controller.
> 
> On the other hand, if this can change somehow dynamically, maybe by the user
> changing a setting, then we have to still register here, since we don't know.

There are two stage for Enabling Arc. IsArcAllowedForProfile() is static through
user session(Only in tests we can change its return value by 
SetArcAvailableCommandLineForTesting for primary profile) If this returns false,
user will have no option to enable Arc at all. The second is whether user
enables Arc, which can be dynamic at runtime. Currently I am using the second
signal to control ReadyForStart() of the data type controller. I think adding
the switch here is safe as IsArcAllowedForProfile will not change within user
session. My previous concern is this function will always return false in sync
integration test for the test profiles. I don't have enough knowledge how you
are still able to create the datatype controller for those test profiles. But if
you already have proper coverage I would like to see the switch here as memory
saving is always preferred.

[Gang Wu, 2017-03-15 17:24:09]

Patchset #3 (id:80001) has been deleted

[Gang Wu, 2017-03-15 17:39:08]

The CQ bit was checked by gangwu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-15 17:39:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Gang Wu, 2017-03-15 17:42:32]

The CQ bit was checked by gangwu@chromium.org to run a CQ dry run

[Gang Wu, 2017-03-15 17:42:47]

Patchset #3 (id:100001) has been deleted

[commit-bot: I haz the power, 2017-03-15 17:42:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Gang Wu, 2017-03-15 17:52:26]

The CQ bit was checked by gangwu@chromium.org to run a CQ dry run

[Gang Wu, 2017-03-15 17:52:40]

Patchset #3 (id:120001) has been deleted

[commit-bot: I haz the power, 2017-03-15 17:52:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Gang Wu, 2017-03-15 17:54:11]

Description was changed from

==========
[Sync] Add switch for ARC_PACKAGE

After add switch, two of sync_integration_tests could be enabled.

BUG=689662
==========

to

==========
[Sync] Fix EnableDisableSingleClientTest

BUG=689662
==========

[commit-bot: I haz the power, 2017-03-15 18:03:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-15 18:03:10]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Gang Wu, 2017-03-15 18:23:11]

The CQ bit was checked by gangwu@chromium.org to run a CQ dry run

[Gang Wu, 2017-03-15 18:23:27]

Patchset #3 (id:140001) has been deleted

[commit-bot: I haz the power, 2017-03-15 18:23:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-15 19:09:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-15 19:09:21]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Gang Wu, 2017-03-15 19:50:43]

The CQ bit was checked by gangwu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-15 19:51:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Gang Wu, 2017-03-15 19:51:27]

Patchset #3 (id:160001) has been deleted

[commit-bot: I haz the power, 2017-03-15 20:56:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-15 20:56:12]

Dry run: This issue passed the CQ dry run.

[Gang Wu, 2017-03-15 23:12:14]

The CQ bit was checked by gangwu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-15 23:12:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Gang Wu, 2017-03-15 23:19:13]

The CQ bit was checked by gangwu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-15 23:19:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Gang Wu, 2017-03-15 23:25:58]

The CQ bit was checked by gangwu@chromium.org to run a CQ dry run

[Gang Wu, 2017-03-15 23:26:23]

Patchset #5 (id:180002) has been deleted

[commit-bot: I haz the power, 2017-03-15 23:26:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Gang Wu, 2017-03-15 23:29:03]

Patchset #4 (id:200001) has been deleted

[Gang Wu, 2017-03-15 23:32:35]

Patchset #4 (id:230001) has been deleted

[Gang Wu, 2017-03-15 23:38:03]

The CQ bit was checked by gangwu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-15 23:38:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-16 00:34:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-16 00:34:48]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Gang Wu, 2017-03-16 15:46:20]

The CQ bit was checked by gangwu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-16 15:47:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-16 16:31:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-16 16:31:34]

Dry run: This issue passed the CQ dry run.

[Gang Wu, 2017-03-16 18:40:30]

updated, PTAL

https://codereview.chromium.org/2740363002/diff/60001/chrome/browser/sync/chr...
File chrome/browser/sync/chrome_sync_client.cc (right):

https://codereview.chromium.org/2740363002/diff/60001/chrome/browser/sync/chr...
chrome/browser/sync/chrome_sync_client.cc:683: if
(arc::IsArcAllowedForProfile(profile_)) {
On 2017/03/15 17:20:58, lgcheng wrote:
> On 2017/03/15 15:45:58, skym wrote:
> > On 2017/03/15 07:55:45, Gang Wu wrote:
> > > On 2017/03/14 21:59:28, lgcheng wrote:
> > > > I am not sure about the test infrastructure.
> > > > But IMO the switch is a little redundant. The todo here is because at
> first
> > > > deployment we use default datatypecontroller and had no control of
> > > > ArcPackageSyncService, start or stop.
> > > > 
> > > > In later update, we overload readyForStart() method in the data
> controller.
> > If
> > > > arc is not allowed the controller will be marked as not ready to start.
> And
> > I
> > > > should remove the TODO here at that moment.
> > > > 
> > > > Also, arc::IsArcAllowedForProfile() returns false for the test
profile(s),
> > and
> > > > we only let readyForStart() to return true for sync integration test. So
> > maybe
> > > > just remove the TODO is better?
> > > > 
> > > 
> > > Add a switch here not just for fixing test, but also for saving memory.
> > > Normally, we only create controller when necessary.
> > > If the data type is not enabled, the controller won't be created. I think
> most
> > > of controllers did this way.
> > > Of course, if you do have reason to always create arc controller in
> chromeos,
> > I
> > > am fine with that, I can fix test in other way. 
> > 
> > I think the piece of information you need to decide if we should gate based
on
> > this flag, is can it change during runtime? If we know at this point, that
> this
> > profile is never going to be allowed to use ARC, we don't want to
> > create/register the controller.
> > 
> > On the other hand, if this can change somehow dynamically, maybe by the user
> > changing a setting, then we have to still register here, since we don't
know.
> 
> There are two stage for Enabling Arc. IsArcAllowedForProfile() is static
through
> user session(Only in tests we can change its return value by 
> SetArcAvailableCommandLineForTesting for primary profile) If this returns
false,
> user will have no option to enable Arc at all. The second is whether user
> enables Arc, which can be dynamic at runtime. Currently I am using the second
> signal to control ReadyForStart() of the data type controller. I think adding
> the switch here is safe as IsArcAllowedForProfile will not change within user
> session. My previous concern is this function will always return false in sync
> integration test for the test profiles. I don't have enough knowledge how you
> are still able to create the datatype controller for those test profiles. But
if
> you already have proper coverage I would like to see the switch here as memory
> saving is always preferred.  

If user can enable Arc at runtime, I think it is good to register controller
here.

https://codereview.chromium.org/2740363002/diff/60001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/enable_disable_test.cc (right):

https://codereview.chromium.org/2740363002/diff/60001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/enable_disable_test.cc:161:
IN_PROC_BROWSER_TEST_F(EnableDisableSingleClientTest, EnableOneAtATime) {
On 2017/03/15 15:45:58, skym wrote:
> On 2017/03/15 07:55:45, Gang Wu wrote:
> > On 2017/03/14 21:39:06, skym wrote:
> > > Why are these tests being fixed by this change? Am I correct to assume
that
> > > arc::IsArcAllowedForProfile() returns false for these two tests? Wouldn't
we
> > > prefer these tests to include ARC_PACKAGE? Is there some setup call we
need
> to
> > > make to make these work instead?
> > 
> > Before this change, sync client always create ARC controller whenever ARC is
> > enabled or not. SyncService has the controller, but client cannot enable the
> ARC
> > service.
> > After this change, client will only create controller when service can be
> > enabled.
> > 
> 
> What I was trying to say is that you're fixing these tests by not registering
> the controller. I'd prefer you to fix them by registering the controller and
> enabling the ARC machinery.
> 
> I have nothing against the selective registration of the ARC controller, but I
> want these tests to register it.

Done.

https://codereview.chromium.org/2740363002/diff/270001/chrome/browser/sync/te...
File chrome/browser/sync/test/integration/sync_test.cc (right):

https://codereview.chromium.org/2740363002/diff/270001/chrome/browser/sync/te...
chrome/browser/sync/test/integration/sync_test.cc:303: if
(!cl->HasSwitch(switches::kSupervisedUserId))
add this because "Supervised users are not supported in ARC." from
arc_util.cc:59,

[lgcheng, 2017-03-16 23:06:12]

On 2017/03/16 18:40:30, Gang Wu wrote:
> updated, PTAL
> 
>
https://codereview.chromium.org/2740363002/diff/60001/chrome/browser/sync/chr...
> File chrome/browser/sync/chrome_sync_client.cc (right):
> 
>
https://codereview.chromium.org/2740363002/diff/60001/chrome/browser/sync/chr...
> chrome/browser/sync/chrome_sync_client.cc:683: if
> (arc::IsArcAllowedForProfile(profile_)) {
> On 2017/03/15 17:20:58, lgcheng wrote:
> > On 2017/03/15 15:45:58, skym wrote:
> > > On 2017/03/15 07:55:45, Gang Wu wrote:
> > > > On 2017/03/14 21:59:28, lgcheng wrote:
> > > > > I am not sure about the test infrastructure.
> > > > > But IMO the switch is a little redundant. The todo here is because at
> > first
> > > > > deployment we use default datatypecontroller and had no control of
> > > > > ArcPackageSyncService, start or stop.
> > > > > 
> > > > > In later update, we overload readyForStart() method in the data
> > controller.
> > > If
> > > > > arc is not allowed the controller will be marked as not ready to
start.
> > And
> > > I
> > > > > should remove the TODO here at that moment.
> > > > > 
> > > > > Also, arc::IsArcAllowedForProfile() returns false for the test
> profile(s),
> > > and
> > > > > we only let readyForStart() to return true for sync integration test.
So
> > > maybe
> > > > > just remove the TODO is better?
> > > > > 
> > > > 
> > > > Add a switch here not just for fixing test, but also for saving memory.
> > > > Normally, we only create controller when necessary.
> > > > If the data type is not enabled, the controller won't be created. I
think
> > most
> > > > of controllers did this way.
> > > > Of course, if you do have reason to always create arc controller in
> > chromeos,
> > > I
> > > > am fine with that, I can fix test in other way. 
> > > 
> > > I think the piece of information you need to decide if we should gate
based
> on
> > > this flag, is can it change during runtime? If we know at this point, that
> > this
> > > profile is never going to be allowed to use ARC, we don't want to
> > > create/register the controller.
> > > 
> > > On the other hand, if this can change somehow dynamically, maybe by the
user
> > > changing a setting, then we have to still register here, since we don't
> know.
> > 
> > There are two stage for Enabling Arc. IsArcAllowedForProfile() is static
> through
> > user session(Only in tests we can change its return value by 
> > SetArcAvailableCommandLineForTesting for primary profile) If this returns
> false,
> > user will have no option to enable Arc at all. The second is whether user
> > enables Arc, which can be dynamic at runtime. Currently I am using the
second
> > signal to control ReadyForStart() of the data type controller. I think
adding
> > the switch here is safe as IsArcAllowedForProfile will not change within
user
> > session. My previous concern is this function will always return false in
sync
> > integration test for the test profiles. I don't have enough knowledge how
you
> > are still able to create the datatype controller for those test profiles.
But
> if
> > you already have proper coverage I would like to see the switch here as
memory
> > saving is always preferred.  
> 
> If user can enable Arc at runtime, I think it is good to register controller
> here.
> 
>
https://codereview.chromium.org/2740363002/diff/60001/chrome/browser/sync/tes...
> File chrome/browser/sync/test/integration/enable_disable_test.cc (right):
> 
>
https://codereview.chromium.org/2740363002/diff/60001/chrome/browser/sync/tes...
> chrome/browser/sync/test/integration/enable_disable_test.cc:161:
> IN_PROC_BROWSER_TEST_F(EnableDisableSingleClientTest, EnableOneAtATime) {
> On 2017/03/15 15:45:58, skym wrote:
> > On 2017/03/15 07:55:45, Gang Wu wrote:
> > > On 2017/03/14 21:39:06, skym wrote:
> > > > Why are these tests being fixed by this change? Am I correct to assume
> that
> > > > arc::IsArcAllowedForProfile() returns false for these two tests?
Wouldn't
> we
> > > > prefer these tests to include ARC_PACKAGE? Is there some setup call we
> need
> > to
> > > > make to make these work instead?
> > > 
> > > Before this change, sync client always create ARC controller whenever ARC
is
> > > enabled or not. SyncService has the controller, but client cannot enable
the
> > ARC
> > > service.
> > > After this change, client will only create controller when service can be
> > > enabled.
> > > 
> > 
> > What I was trying to say is that you're fixing these tests by not
registering
> > the controller. I'd prefer you to fix them by registering the controller and
> > enabling the ARC machinery.
> > 
> > I have nothing against the selective registration of the ARC controller, but
I
> > want these tests to register it.
> 
> Done.
> 
>
https://codereview.chromium.org/2740363002/diff/270001/chrome/browser/sync/te...
> File chrome/browser/sync/test/integration/sync_test.cc (right):
> 
>
https://codereview.chromium.org/2740363002/diff/270001/chrome/browser/sync/te...
> chrome/browser/sync/test/integration/sync_test.cc:303: if
> (!cl->HasSwitch(switches::kSupervisedUserId))
> add this because "Supervised users are not supported in ARC." from
> arc_util.cc:59,
LGTM 

One thing to mention is that moving Arc setup code to sync_test is very neat,
but it may introduce risk to potential test failure on other sync integration
tests on ChromeOS. We can fix that if happens.

Defer to skym@ for approval.

[skym, 2017-03-16 23:27:19]

I wonder if SyncArcPackageHelper really even needs to be a singleton, if it
could just be a bunch of static methods. But I guess that's out of the scope of
this CL.

https://codereview.chromium.org/2740363002/diff/270001/chrome/browser/sync/ch...
File chrome/browser/sync/chrome_sync_client.cc (left):

https://codereview.chromium.org/2740363002/diff/270001/chrome/browser/sync/ch...
chrome/browser/sync/chrome_sync_client.cc:682: // TODO(lgcheng): Add switch for
this.
Why didn't you add the switch back here, Gang? I thought we talked about adding
it back on Wednesday.

https://codereview.chromium.org/2740363002/diff/270001/chrome/browser/sync/te...
File chrome/browser/sync/test/integration/sync_test.cc (right):

https://codereview.chromium.org/2740363002/diff/270001/chrome/browser/sync/te...
chrome/browser/sync/test/integration/sync_test.cc:303: if
(!cl->HasSwitch(switches::kSupervisedUserId))
On 2017/03/16 18:40:30, Gang Wu wrote:
> add this because "Supervised users are not supported in ARC." from
> arc_util.cc:59, 

Can you put that into a comment?

https://codereview.chromium.org/2740363002/diff/270001/chrome/browser/sync/te...
chrome/browser/sync/test/integration/sync_test.cc:553: if
(!cl->HasSwitch(switches::kSupervisedUserId)) {
I've seen this check before! Can you unify them? Also, it'd be interesting to
mention in a comment how early this kSupervisedUserId flag is getting set, and
why this is safe.

https://codereview.chromium.org/2740363002/diff/270001/chrome/browser/sync/te...
File chrome/browser/sync/test/integration/sync_test.h (right):

https://codereview.chromium.org/2740363002/diff/270001/chrome/browser/sync/te...
chrome/browser/sync/test/integration/sync_test.h:487: arc::SyncArcPackageHelper*
sync_arc_helper_;
Am I missing something? Why not call arc::SyncArcPackageHelper::GetInstance()
every time instead of holding onto a non-owning copy?

[Gang Wu, 2017-03-17 06:17:13]

The CQ bit was checked by gangwu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-17 06:17:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-17 06:31:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-17 06:31:08]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Gang Wu, 2017-03-17 06:37:57]

The CQ bit was checked by gangwu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-17 06:38:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Gang Wu, 2017-03-17 06:40:25]

The CQ bit was checked by gangwu@chromium.org to run a CQ dry run

[Gang Wu, 2017-03-17 06:40:39]

Patchset #6 (id:290001) has been deleted

[commit-bot: I haz the power, 2017-03-17 06:40:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Gang Wu, 2017-03-17 06:40:48]

Patchset #6 (id:310001) has been deleted

[commit-bot: I haz the power, 2017-03-17 06:52:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-17 06:52:58]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Gang Wu, 2017-03-17 06:59:39]

The CQ bit was checked by gangwu@chromium.org to run a CQ dry run

[Gang Wu, 2017-03-17 06:59:50]

Patchset #6 (id:330001) has been deleted

[commit-bot: I haz the power, 2017-03-17 07:00:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-17 07:54:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-17 07:54:52]

Dry run: This issue passed the CQ dry run.

[Gang Wu, 2017-03-17 16:50:33]

The CQ bit was checked by gangwu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-17 16:50:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Gang Wu, 2017-03-17 17:01:13]

The CQ bit was checked by gangwu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-17 17:01:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Gang Wu, 2017-03-17 17:01:51]

Patchset #7 (id:370001) has been deleted

[Gang Wu, 2017-03-17 17:09:14]

Patchset #6 (id:350001) has been deleted

[Gang Wu, 2017-03-17 17:38:22]

The CQ bit was checked by gangwu@chromium.org to run a CQ dry run

[Gang Wu, 2017-03-17 17:38:35]

Patchset #6 (id:390001) has been deleted

[commit-bot: I haz the power, 2017-03-17 17:38:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-17 17:49:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-17 17:49:44]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Gang Wu, 2017-03-17 18:11:18]

The CQ bit was checked by gangwu@chromium.org to run a CQ dry run

[Gang Wu, 2017-03-17 18:11:28]

Patchset #6 (id:410001) has been deleted

[commit-bot: I haz the power, 2017-03-17 18:11:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-17 19:10:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-17 19:10:20]

Dry run: This issue passed the CQ dry run.

[Gang Wu, 2017-03-17 19:23:23]

add switch back since I misread the lgcheng's comments.

https://codereview.chromium.org/2740363002/diff/270001/chrome/browser/sync/ch...
File chrome/browser/sync/chrome_sync_client.cc (left):

https://codereview.chromium.org/2740363002/diff/270001/chrome/browser/sync/ch...
chrome/browser/sync/chrome_sync_client.cc:682: // TODO(lgcheng): Add switch for
this.
On 2017/03/16 23:27:19, skym wrote:
> Why didn't you add the switch back here, Gang? I thought we talked about
adding
> it back on Wednesday.

Done.

https://codereview.chromium.org/2740363002/diff/270001/chrome/browser/sync/te...
File chrome/browser/sync/test/integration/sync_test.cc (right):

https://codereview.chromium.org/2740363002/diff/270001/chrome/browser/sync/te...
chrome/browser/sync/test/integration/sync_test.cc:303: if
(!cl->HasSwitch(switches::kSupervisedUserId))
On 2017/03/16 23:27:19, skym wrote:
> On 2017/03/16 18:40:30, Gang Wu wrote:
> > add this because "Supervised users are not supported in ARC." from
> > arc_util.cc:59, 
> 
> Can you put that into a comment?

Done.

https://codereview.chromium.org/2740363002/diff/270001/chrome/browser/sync/te...
chrome/browser/sync/test/integration/sync_test.cc:553: if
(!cl->HasSwitch(switches::kSupervisedUserId)) {
On 2017/03/16 23:27:19, skym wrote:
> I've seen this check before! Can you unify them? Also, it'd be interesting to
> mention in a comment how early this kSupervisedUserId flag is getting set, and
> why this is safe.

cannot combine two code together,
ArcAppListPrefsFactory::SetFactoryForSyncTest() need to be called before create
profile, and create helper need to be called after profile set up. but I move
them closer to easier understand.

https://codereview.chromium.org/2740363002/diff/270001/chrome/browser/sync/te...
File chrome/browser/sync/test/integration/sync_test.h (right):

https://codereview.chromium.org/2740363002/diff/270001/chrome/browser/sync/te...
chrome/browser/sync/test/integration/sync_test.h:487: arc::SyncArcPackageHelper*
sync_arc_helper_;
On 2017/03/16 23:27:19, skym wrote:
> Am I missing something? Why not call arc::SyncArcPackageHelper::GetInstance()
> every time instead of holding onto a non-owning copy?

Done.

[skym, 2017-03-17 19:51:17]

lgtm

Thanks for all the updates Gang!

https://codereview.chromium.org/2740363002/diff/430001/chrome/browser/sync/te...
File chrome/browser/sync/test/integration/sync_test.cc (right):

https://codereview.chromium.org/2740363002/diff/430001/chrome/browser/sync/te...
chrome/browser/sync/test/integration/sync_test.cc:524: if
(!cl->HasSwitch(switches::kSupervisedUserId))
can you add a { } here, the comment makes this weird to not have { }

[Gang Wu, 2017-03-17 19:59:39]

The CQ bit was checked by gangwu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-17 20:00:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Gang Wu, 2017-03-17 20:00:30]

https://codereview.chromium.org/2740363002/diff/430001/chrome/browser/sync/te...
File chrome/browser/sync/test/integration/sync_test.cc (right):

https://codereview.chromium.org/2740363002/diff/430001/chrome/browser/sync/te...
chrome/browser/sync/test/integration/sync_test.cc:524: if
(!cl->HasSwitch(switches::kSupervisedUserId))
On 2017/03/17 19:51:17, skym wrote:
> can you add a { } here, the comment makes this weird to not have { }

Done.

[commit-bot: I haz the power, 2017-03-17 21:20:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-17 21:20:19]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Gang Wu, 2017-03-17 21:27:44]

The CQ bit was checked by gangwu@chromium.org

[Gang Wu, 2017-03-17 21:27:44]

The patchset sent to the CQ was uploaded after l-g-t-m from lgcheng@google.com,
skym@chromium.org
Link to the patchset: https://codereview.chromium.org/2740363002/#ps450001
(title: "add {}")

[commit-bot: I haz the power, 2017-03-17 21:28:28]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-17 22:12:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-17 22:12:08]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Gang Wu, 2017-03-18 05:11:02]

The CQ bit was checked by gangwu@chromium.org

[commit-bot: I haz the power, 2017-03-18 05:11:12]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-18 05:15:42]

CQ is committing da patch.

Bot data: {"patchset_id": 450001, "attempt_start_ts": 1489813862359800,
"parent_rev": "4bed6671a93fb6d88d49cb8cd4136eb0257f1751", "commit_rev":
"3f8c8d163c769d9b890e8eeb29d1ea1001e312ad"}

[commit-bot: I haz the power, 2017-03-18 05:16:45]

Description was changed from

==========
[Sync] Fix EnableDisableSingleClientTest

BUG=689662
==========

to

==========
[Sync] Fix EnableDisableSingleClientTest

BUG=689662

Review-Url: https://codereview.chromium.org/2740363002
Cr-Commit-Position: refs/heads/master@{#457959}
Committed:
https://chromium.googlesource.com/chromium/src/+/3f8c8d163c769d9b890e8eeb29d1...
==========

[commit-bot: I haz the power, 2017-03-18 05:16:46]

Committed patchset #7 (id:450001) as
https://chromium.googlesource.com/chromium/src/+/3f8c8d163c769d9b890e8eeb29d1...