[Sync] Fix EnableDisableSingleClientTest

chrome/browser/sync/chrome_sync_client.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/sync/chrome_sync_client.h"
 
 #include <utility>
 
 #include "base/bind.h"
 #include "base/command_line.h"
@@ skipping 90 matching lines @@
 #if BUILDFLAG(ENABLE_SPELLCHECK)
 #include "chrome/browser/spellchecker/spellcheck_factory.h"
 #include "chrome/browser/spellchecker/spellcheck_service.h"
 #endif
 
 #if defined(OS_ANDROID)
 #include "chrome/browser/sync/glue/synced_window_delegates_getter_android.h"
 #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"
 #include "chrome/browser/ui/app_list/arc/arc_package_sync_data_type_controller.h"
 #include "chrome/browser/ui/app_list/arc/arc_package_syncable_service.h"
 #include "components/sync_wifi/wifi_credential_syncable_service.h"
 #include "components/sync_wifi/wifi_credential_syncable_service_factory.h"
 #endif
 
 using content::BrowserThread;
@@ skipping 551 matching lines @@
 #if defined(OS_CHROMEOS)
   if (base::CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kEnableWifiCredentialSync) &&
       !disabled_types.Has(syncer::WIFI_CREDENTIALS)) {
     sync_service->RegisterDataTypeController(
         base::MakeUnique<AsyncDirectoryTypeController>(
             syncer::WIFI_CREDENTIALS, error_callback, this, syncer::GROUP_UI,
             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
 }
 
 void ChromeSyncClient::RegisterAndroidDataTypes(
     syncer::SyncService* sync_service,
     syncer::ModelTypeSet disabled_types,
     syncer::ModelTypeSet enabled_types) {
   base::Closure error_callback =
       base::Bind(&syncer::ReportUnrecoverableError, chrome::GetChannel());
 #if BUILDFLAG(ENABLE_SUPERVISED_USERS)
   sync_service->RegisterDataTypeController(
       base::MakeUnique<SupervisedUserSyncDataTypeController>(
           syncer::SUPERVISED_USER_SETTINGS, error_callback, this, profile_));
   sync_service->RegisterDataTypeController(
       base::MakeUnique<SupervisedUserSyncDataTypeController>(
           syncer::SUPERVISED_USER_WHITELISTS, error_callback, this, profile_));
 #endif
 }
 
 }  // namespace browser_sync