sync: Add Wi-Fi configurations sync to OS sync settings page

Move the Wi-Fi configurations to UserSelectableOsType. Change the
ModelTypeController to respect the global "OS sync enabled" pref.
Refactor the printers ModelTypeController to share code.

Note: This means Wi-Fi config sync cannot ship before Split Settings
Sync.  However, it's much simpler than trying to support Wi-Fi config
being *both* a UserSelectableType and a UserSelectableOsType.
I've discussed this with the engineer working on Wi-Fi sync and we're
both OK with this plan.

Bug: 1013466
Test: existing components_unittests and unit_tests
Test: Run --enable-features=SplitSettingsSync,SyncWifiConfigurations
  and verify "Wi-Fi configurations" appears in Chrome OS settings.
  Verify turning it off and closing settings results in
  chrome://sync-internals showing that the Wi-Fi type is not syncing.

Change-Id: I878fecaf449248ae8747e37aa2e0e897130e63ad
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1947396Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Commit-Queue: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#721541}

chrome/browser/chromeos/BUILD.gn

@@ -2053,8 +2053,6 @@ source_set("chromeos") {
     "printing/printer_metrics_provider.h",
     "printing/printers_map.cc",
     "printing/printers_map.h",
-    "printing/printers_model_type_controller.cc",
-    "printing/printers_model_type_controller.h",
     "printing/printers_sync_bridge.cc",
     "printing/printers_sync_bridge.h",
     "printing/server_printers_fetcher.cc",
@@ -2164,6 +2162,8 @@ source_set("chromeos") {
     "startup_settings_cache.h",
     "sync/os_preferences_model_type_controller.cc",
     "sync/os_preferences_model_type_controller.h",
+    "sync/os_sync_model_type_controller.cc",
+    "sync/os_sync_model_type_controller.h",
     "system/automatic_reboot_manager.cc",
     "system/automatic_reboot_manager.h",
     "system/automatic_reboot_manager_observer.h",

chrome/browser/chromeos/printing/printers_model_type_controller.cc → chrome/browser/chromeos/sync/os_sync_model_type_controller.cc

@@ -2,8 +2,9 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "chrome/browser/chromeos/printing/printers_model_type_controller.h"
+#include "chrome/browser/chromeos/sync/os_sync_model_type_controller.h"
 
+#include <memory>
 #include <utility>
 
 #include "base/bind.h"
@@ -15,11 +16,12 @@
 #include "components/sync/driver/sync_service.h"
 #include "components/sync/model/model_type_controller_delegate.h"
 
-PrintersModelTypeController::PrintersModelTypeController(
+OsSyncModelTypeController::OsSyncModelTypeController(
+    syncer::ModelType type,
     std::unique_ptr<syncer::ModelTypeControllerDelegate> delegate,
     PrefService* pref_service,
     syncer::SyncService* sync_service)
-    : syncer::ModelTypeController(syncer::PRINTERS, std::move(delegate)),
+    : syncer::ModelTypeController(type, std::move(delegate)),
       pref_service_(pref_service),
       sync_service_(sync_service) {
   DCHECK(chromeos::features::IsSplitSettingsSyncEnabled());
@@ -28,21 +30,21 @@ PrintersModelTypeController::PrintersModelTypeController(
   pref_registrar_.Init(pref_service_);
   pref_registrar_.Add(
       syncer::prefs::kOsSyncFeatureEnabled,
-      base::BindRepeating(&PrintersModelTypeController::OnUserPrefChanged,
+      base::BindRepeating(&OsSyncModelTypeController::OnUserPrefChanged,
                           base::Unretained(this)));
 }
 
-PrintersModelTypeController::~PrintersModelTypeController() = default;
+OsSyncModelTypeController::~OsSyncModelTypeController() = default;
 
 syncer::DataTypeController::PreconditionState
-PrintersModelTypeController::GetPreconditionState() const {
+OsSyncModelTypeController::GetPreconditionState() const {
   DCHECK(CalledOnValidThread());
   return pref_service_->GetBoolean(syncer::prefs::kOsSyncFeatureEnabled)
              ? PreconditionState::kPreconditionsMet
              : PreconditionState::kMustStopAndClearData;
 }
 
-void PrintersModelTypeController::OnUserPrefChanged() {
+void OsSyncModelTypeController::OnUserPrefChanged() {
   DCHECK(CalledOnValidThread());
   sync_service_->DataTypePreconditionChanged(type());
 }

chrome/browser/chromeos/printing/printers_model_type_controller.h → chrome/browser/chromeos/sync/os_sync_model_type_controller.h

@@ -2,8 +2,10 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef CHROME_BROWSER_CHROMEOS_PRINTING_PRINTERS_MODEL_TYPE_CONTROLLER_H_
-#define CHROME_BROWSER_CHROMEOS_PRINTING_PRINTERS_MODEL_TYPE_CONTROLLER_H_
+#ifndef CHROME_BROWSER_CHROMEOS_SYNC_OS_SYNC_MODEL_TYPE_CONTROLLER_H_
+#define CHROME_BROWSER_CHROMEOS_SYNC_OS_SYNC_MODEL_TYPE_CONTROLLER_H_
+
+#include <memory>
 
 #include "components/prefs/pref_change_registrar.h"
 #include "components/sync/driver/model_type_controller.h"
@@ -15,17 +17,19 @@ class ModelTypeControllerDelegate;
 class SyncService;
 }  // namespace syncer
 
-// Controls syncing of ModelType PRINTERS.
-class PrintersModelTypeController : public syncer::ModelTypeController {
+// Controls sync of Chrome OS ModelTypes that depend on the system-wide
+// kOsSyncFeatureEnabled preference.
+class OsSyncModelTypeController : public syncer::ModelTypeController {
  public:
-  PrintersModelTypeController(
+  OsSyncModelTypeController(
+      syncer::ModelType type,
       std::unique_ptr<syncer::ModelTypeControllerDelegate> delegate,
       PrefService* pref_service,
       syncer::SyncService* sync_service);
-  ~PrintersModelTypeController() override;
+  ~OsSyncModelTypeController() override;
 
-  PrintersModelTypeController(const PrintersModelTypeController&) = delete;
-  PrintersModelTypeController& operator=(const PrintersModelTypeController&) =
+  OsSyncModelTypeController(const OsSyncModelTypeController&) = delete;
+  OsSyncModelTypeController& operator=(const OsSyncModelTypeController&) =
       delete;
 
   // DataTypeController:
@@ -41,4 +45,4 @@ class PrintersModelTypeController : public syncer::ModelTypeController {
   PrefChangeRegistrar pref_registrar_;
 };
 
-#endif  // CHROME_BROWSER_CHROMEOS_PRINTING_PRINTERS_MODEL_TYPE_CONTROLLER_H_
+#endif  // CHROME_BROWSER_CHROMEOS_SYNC_OS_SYNC_MODEL_TYPE_CONTROLLER_H_

chrome/browser/resources/settings/chromeos/os_people_page/os_sync_browser_proxy.js

@@ -25,6 +25,9 @@ cr.exportPath('settings');
  *   printersRegistered: boolean,
  *   printersSynced: boolean,
  *   syncAllOsDataTypes: boolean,
+ *   wifiConfigurationsEnforced: boolean,
+ *   wifiConfigurationsRegistered: boolean,
+ *   wifiConfigurationsSynced: boolean,
  * }}
  */
 settings.OsSyncPrefs;

chrome/browser/resources/settings/chromeos/os_people_page/os_sync_controls.html

@@ -86,7 +86,21 @@
         </cr-toggle>
       </div>
 
-      <!-- TODO(jamescook): Wallpaper, Wi-Fi configs. -->
+      <div class="list-item"
+           hidden="[[!osSyncPrefs.wifiConfigurationsRegistered]]">
+        <div id="wifiConfigurationsCheckboxLabel">
+          $i18n{wifiConfigurationsCheckboxLabel}
+        </div>
+        <cr-toggle checked="{{osSyncPrefs.wifiConfigurationsSynced}}"
+            on-change="onSingleSyncDataTypeChanged_"
+            disabled="[[shouldSyncCheckboxBeDisabled_(
+                osSyncPrefs.syncAllOsTypes,
+                osSyncPrefs.wifiConfigurationsEnforced)]]"
+            aria-labelledby="wifiConfigurationsCheckboxLabel">
+        </cr-toggle>
+      </div>
+
+      <!-- TODO(jamescook): Wallpaper. -->
 
     </div>
   </template>

chrome/browser/resources/settings/chromeos/os_people_page/os_sync_controls.js

@@ -13,6 +13,7 @@ const SyncPrefsIndividualDataTypes = [
   'osAppsSynced',
   'osPreferencesSynced',
   'printersSynced',
+  'wifiConfigurationsSynced',
 ];
 
 /**

chrome/browser/resources/settings/people_page/sync_browser_proxy.js

@@ -100,9 +100,6 @@ settings.StatusAction = {
  *   typedUrlsEnforced: boolean,
  *   typedUrlsRegistered: boolean,
  *   typedUrlsSynced: boolean,
- *   wifiConfigurationsEnforced: boolean,
- *   wifiConfigurationsRegistered: boolean,
- *   wifiConfigurationsSynced: boolean,
  * }}
  */
 settings.SyncPrefs;

chrome/browser/resources/settings/people_page/sync_controls.html

@@ -135,22 +135,6 @@
         </cr-toggle>
       </div>
 
-<if expr="chromeos">
-      <div class="list-item"
-           hidden="[[!syncPrefs.wifiConfigurationsRegistered]]">
-        <div id="wifiConfigurationsCheckboxLabel">
-          $i18n{wifiConfigurationsCheckboxLabel}
-        </div>
-        <cr-toggle checked="{{syncPrefs.wifiConfigurationsSynced}}"
-            on-change="onSingleSyncDataTypeChanged_"
-            disabled="[[shouldSyncCheckboxBeDisabled_(
-                syncPrefs.syncAllDataTypes,
-                syncPrefs.wifiConfigurationsEnforced)]]"
-            aria-labelledby="wifiConfigurationsCheckboxLabel">
-        </cr-toggle>
-      </div>
-</if>
-
       <div class="list-item" hidden="[[!syncPrefs.autofillRegistered]]">
         <div id="autofillCheckboxLabel">
           $i18n{autofillCheckboxLabel}

chrome/browser/sync/chrome_sync_client.cc

@@ -115,11 +115,11 @@
 #if defined(OS_CHROMEOS)
 #include "ash/public/cpp/app_list/app_list_switches.h"
 #include "chrome/browser/chromeos/arc/arc_util.h"
-#include "chrome/browser/chromeos/printing/printers_model_type_controller.h"
 #include "chrome/browser/chromeos/printing/printers_sync_bridge.h"
 #include "chrome/browser/chromeos/printing/synced_printers_manager.h"
 #include "chrome/browser/chromeos/printing/synced_printers_manager_factory.h"
 #include "chrome/browser/chromeos/sync/os_preferences_model_type_controller.h"
+#include "chrome/browser/chromeos/sync/os_sync_model_type_controller.h"
 #include "chrome/browser/sync/wifi_configuration_sync_service_factory.h"
 #include "chrome/browser/ui/app_list/app_list_syncable_service.h"
 #include "chrome/browser/ui/app_list/app_list_syncable_service_factory.h"
@@ -440,11 +440,21 @@ ChromeSyncClient::CreateDataTypeControllers(syncer::SyncService* sync_service) {
           dump_stack, profile_->GetPrefs(), sync_service));
     }
     if (!disabled_types.Has(syncer::PRINTERS)) {
-      controllers.push_back(std::make_unique<PrintersModelTypeController>(
+      controllers.push_back(std::make_unique<OsSyncModelTypeController>(
+          syncer::PRINTERS,
           std::make_unique<syncer::ForwardingModelTypeControllerDelegate>(
               GetControllerDelegateForModelType(syncer::PRINTERS).get()),
           profile_->GetPrefs(), sync_service));
     }
+    if (!disabled_types.Has(syncer::WIFI_CONFIGURATIONS) &&
+        base::FeatureList::IsEnabled(switches::kSyncWifiConfigurations)) {
+      controllers.push_back(std::make_unique<OsSyncModelTypeController>(
+          syncer::WIFI_CONFIGURATIONS,
+          std::make_unique<syncer::ForwardingModelTypeControllerDelegate>(
+              GetControllerDelegateForModelType(syncer::WIFI_CONFIGURATIONS)
+                  .get()),
+          profile_->GetPrefs(), sync_service));
+    }
   }
 #endif  // defined(OS_CHROMEOS)
 

chrome/browser/ui/webui/chromeos/sync/os_sync_handler_unittest.cc

@@ -55,6 +55,8 @@ DictionaryValue CreateOsSyncPrefs(FeatureConfig feature,
                     types.Has(UserSelectableOsType::kOsPreferences));
   result.SetBoolean("printersSynced",
                     types.Has(UserSelectableOsType::kPrinters));
+  result.SetBoolean("wifiConfigurationsSynced",
+                    types.Has(UserSelectableOsType::kWifiConfigurations));
   return result;
 }
 
@@ -81,6 +83,8 @@ void CheckConfigDataTypeArguments(const DictionaryValue* dictionary,
             types.Has(UserSelectableOsType::kOsPreferences));
   CheckBool(dictionary, "printersSynced",
             types.Has(UserSelectableOsType::kPrinters));
+  CheckBool(dictionary, "wifiConfigurationsSynced",
+            types.Has(UserSelectableOsType::kWifiConfigurations));
 }
 
 std::unique_ptr<KeyedService> BuildMockSyncService(
@@ -290,6 +294,7 @@ TEST_F(OsSyncHandlerTest, ShowSetupSyncEverything) {
   CheckBool(dictionary, "syncAllOsTypes", true);
   CheckBool(dictionary, "osPreferencesRegistered", true);
   CheckBool(dictionary, "printersRegistered", true);
+  CheckBool(dictionary, "wifiConfigurationsRegistered", true);
   CheckConfigDataTypeArguments(dictionary, SYNC_ALL_OS_TYPES,
                                UserSelectableOsTypeSet::All());
 }

chrome/browser/ui/webui/settings/people_handler_unittest.cc

@@ -103,8 +103,6 @@ std::string GetConfiguration(const base::DictionaryValue* extra_values,
                     types.Has(syncer::UserSelectableType::kExtensions));
   result.SetBoolean("passwordsSynced",
                     types.Has(syncer::UserSelectableType::kPasswords));
-  result.SetBoolean("wifiConfigurationsSynced",
-                    types.Has(syncer::UserSelectableType::kWifiConfigurations));
   result.SetBoolean("preferencesSynced",
                     types.Has(syncer::UserSelectableType::kPreferences));
   result.SetBoolean("tabsSynced", types.Has(syncer::UserSelectableType::kTabs));
@@ -165,8 +163,6 @@ void CheckConfigDataTypeArguments(const base::DictionaryValue* dictionary,
             types.Has(syncer::UserSelectableType::kExtensions));
   CheckBool(dictionary, "passwordsSynced",
             types.Has(syncer::UserSelectableType::kPasswords));
-  CheckBool(dictionary, "wifiConfigurationsSynced",
-            types.Has(syncer::UserSelectableType::kWifiConfigurations));
   CheckBool(dictionary, "preferencesSynced",
             types.Has(syncer::UserSelectableType::kPreferences));
   CheckBool(dictionary, "tabsSynced",
@@ -856,7 +852,6 @@ TEST_F(PeopleHandlerTest, ShowSetupSyncEverything) {
   CheckBool(dictionary, "bookmarksRegistered", true);
   CheckBool(dictionary, "extensionsRegistered", true);
   CheckBool(dictionary, "passwordsRegistered", true);
-  CheckBool(dictionary, "wifiConfigurationsRegistered", true);
   CheckBool(dictionary, "preferencesRegistered", true);
   CheckBool(dictionary, "tabsRegistered", true);
   CheckBool(dictionary, "themesRegistered", true);

chrome/test/data/webui/settings/chromeos/os_sync_controls_test.js

@@ -44,6 +44,9 @@ function getSyncAllOsPrefs() {
     printersRegistered: true,
     printersSynced: true,
     syncAllOsTypes: true,
+    wifiConfigurationsEnforced: false,
+    wifiConfigurationsRegistered: true,
+    wifiConfigurationsSynced: true,
   };
 }
 

components/browser_sync/profile_sync_components_factory_impl.cc

@@ -321,21 +321,12 @@ ProfileSyncComponentsFactoryImpl::CreateCommonDataTypeControllers(
 
 #if defined(OS_CHROMEOS)
   // When SplitSettingsSync is enabled the controller is created in
-  // ChromeSyncClient so it can live near other printer-related sync code.
+  // ChromeSyncClient.
   if (!disabled_types.Has(syncer::PRINTERS) &&
       !chromeos::features::IsSplitSettingsSyncEnabled()) {
     controllers.push_back(
         CreateModelTypeControllerForModelRunningOnUIThread(syncer::PRINTERS));
   }
-  if (!disabled_types.Has(syncer::WIFI_CONFIGURATIONS) &&
-      base::FeatureList::IsEnabled(switches::kSyncWifiConfigurations)) {
-    controllers.push_back(std::make_unique<ModelTypeController>(
-        syncer::WIFI_CONFIGURATIONS,
-        std::make_unique<syncer::ForwardingModelTypeControllerDelegate>(
-            sync_client_
-                ->GetControllerDelegateForModelType(syncer::WIFI_CONFIGURATIONS)
-                .get())));
-  }
 #endif  // defined(OS_CHROMEOS)
 
   // Reading list sync is enabled by default only on iOS. Register unless

components/sync/base/sync_prefs.cc

@@ -134,6 +134,8 @@ const char* GetPrefNameForOsType(UserSelectableOsType type) {
       return prefs::kSyncOsPreferences;
     case UserSelectableOsType::kPrinters:
       return prefs::kSyncOsPrinters;
+    case UserSelectableOsType::kWifiConfigurations:
+      return prefs::kSyncWifiConfigurations;
   }
   NOTREACHED();
   return nullptr;
@@ -591,8 +593,6 @@ const char* SyncPrefs::GetPrefNameForType(UserSelectableType type) {
       return prefs::kSyncReadingList;
     case UserSelectableType::kTabs:
       return prefs::kSyncTabs;
-    case UserSelectableType::kWifiConfigurations:
-      return prefs::kSyncWifiConfigurations;
   }
   NOTREACHED();
   return nullptr;

components/sync/base/user_selectable_type.cc

@@ -34,7 +34,6 @@ constexpr char kExtensionsTypeName[] = "extensions";
 constexpr char kAppsTypeName[] = "apps";
 constexpr char kReadingListTypeName[] = "readingList";
 constexpr char kTabsTypeName[] = "tabs";
-constexpr char kWifiConfigurationsTypeName[] = "wifiConfigurations";
 
 UserSelectableTypeInfo GetUserSelectableTypeInfo(UserSelectableType type) {
   // UserSelectableTypeInfo::type_name is used in js code and shouldn't be
@@ -87,10 +86,6 @@ UserSelectableTypeInfo GetUserSelectableTypeInfo(UserSelectableType type) {
               PROXY_TABS,
               {PROXY_TABS, SESSIONS, FAVICON_IMAGES, FAVICON_TRACKING,
                SEND_TAB_TO_SELF}};
-    case UserSelectableType::kWifiConfigurations:
-      return {kWifiConfigurationsTypeName,
-              WIFI_CONFIGURATIONS,
-              {WIFI_CONFIGURATIONS}};
   }
   NOTREACHED();
   return {nullptr, UNSPECIFIED};
@@ -110,6 +105,8 @@ UserSelectableTypeInfo GetUserSelectableOsTypeInfo(UserSelectableOsType type) {
               {OS_PREFERENCES, OS_PRIORITY_PREFERENCES}};
     case UserSelectableOsType::kPrinters:
       return {"printers", PRINTERS, {PRINTERS}};
+    case UserSelectableOsType::kWifiConfigurations:
+      return {"wifiConfigurations", WIFI_CONFIGURATIONS, {WIFI_CONFIGURATIONS}};
   }
 }
 #endif
@@ -151,9 +148,6 @@ UserSelectableType GetUserSelectableTypeFromString(const std::string& type) {
   if (type == kTabsTypeName) {
     return UserSelectableType::kTabs;
   }
-  if (type == kWifiConfigurationsTypeName) {
-    return UserSelectableType::kWifiConfigurations;
-  }
   NOTREACHED();
   return UserSelectableType::kLastType;
 }

components/sync/base/user_selectable_type.h

@@ -24,7 +24,6 @@ enum class UserSelectableType {
   kExtensions,
   kApps,
   kReadingList,
-  kWifiConfigurations,
   kTabs,
   kLastType = kTabs
 };
@@ -53,7 +52,8 @@ enum class UserSelectableOsType {
 
   kOsPreferences,
   kPrinters,
-  kLastType = kPrinters
+  kWifiConfigurations,
+  kLastType = kWifiConfigurations
 };
 
 using UserSelectableOsTypeSet = EnumSet<UserSelectableOsType,

components/sync/driver/sync_user_settings_unittest.cc

@@ -32,13 +32,16 @@ const char kSyncSessions[] = "sync.sessions";
 ModelTypeSet GetUserTypes() {
   ModelTypeSet user_types = UserTypes();
 #if defined(OS_CHROMEOS)
-  // OS_PREFERENCES only exists when SplitSettingsSync is enabled.
-  if (!chromeos::features::IsSplitSettingsSyncEnabled())
-    user_types.RemoveAll({OS_PREFERENCES, OS_PRIORITY_PREFERENCES});
+  // These types only exist when SplitSettingsSync is enabled.
+  if (!chromeos::features::IsSplitSettingsSyncEnabled()) {
+    user_types.RemoveAll(
+        {OS_PREFERENCES, OS_PRIORITY_PREFERENCES, WIFI_CONFIGURATIONS});
+  }
 #else
   // Ignore all Chrome OS types on non-Chrome OS platforms.
   user_types.RemoveAll({APP_LIST, ARC_PACKAGE, OS_PREFERENCES,
-                        OS_PRIORITY_PREFERENCES, PRINTERS});
+                        OS_PRIORITY_PREFERENCES, PRINTERS,
+                        WIFI_CONFIGURATIONS});
 #endif
   return user_types;
 }