[Sync] Fix EnableDisableSingleClientTest

chrome/browser/sync/chrome_sync_client.cc

Index: chrome/browser/sync/chrome_sync_client.cc
diff --git a/chrome/browser/sync/chrome_sync_client.cc b/chrome/browser/sync/chrome_sync_client.cc
index 2571a605ca9bc29a0994bf68abbce6b70ff88f52..7daf2b925897267483eb02ff849cda75f7c05fd9 100644
--- a/chrome/browser/sync/chrome_sync_client.cc
+++ b/chrome/browser/sync/chrome_sync_client.cc
@@ -108,6 +108,7 @@
 #endif
 
 #if defined(OS_CHROMEOS)
+#include "chrome/browser/chromeos/arc/arc_util.h"
 #include "chrome/browser/chromeos/printing/printers_manager.h"
 #include "chrome/browser/chromeos/printing/printers_manager_factory.h"
 #include "chrome/browser/chromeos/printing/printers_sync_bridge.h"
@@ -679,10 +680,11 @@ void ChromeSyncClient::RegisterDesktopDataTypes(
             BrowserThread::GetTaskRunnerForThread(BrowserThread::UI)));
   }
 
-  // TODO(lgcheng): Add switch for this.
-  sync_service->RegisterDataTypeController(
-      base::MakeUnique<ArcPackageSyncDataTypeController>(
-          syncer::ARC_PACKAGE, error_callback, this, profile_));
+  if (arc::IsArcAllowedForProfile(profile_)) {

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

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:45]

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. 

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

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.

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

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/16 18:40:30]

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

+    sync_service->RegisterDataTypeController(
+        base::MakeUnique<ArcPackageSyncDataTypeController>(
+            syncer::ARC_PACKAGE, error_callback, this, profile_));
+  }
 #endif
 }