chromeos: Sync APP_LIST when Chrome app or ARC app sync is enabled

The SplitSettingsSync feature makes it possible to independently
sync Chrome apps (via browser sync) and ARC apps (via OS sync).
The app list data model and UI code can handle references to
missing apps (for example, on Chromebooks that don't support ARC++
the app list data model may have references to ARC++ apps from
sync, but the UI gracefully hides them).

I initially tied the APP_LIST data type to OS apps. However, the
user can end up in an odd state if we sync Chrome apps
without syncing the app list metadata. For example, a user could
disable OS sync, enable browser sync, install some Chrome
apps, move them around in the app list, set up a new Chromebook,
then discover that the apps are in the wrong order or are not
pinned to the shelf.

Ensure we sync the metadata if either browser apps or OS apps are
being synced.

Bug: 1013466
Test: added to sync_integration_tests

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

chrome/browser/sync/test/integration/single_client_app_list_sync_test.cc

@@ -8,6 +8,8 @@
 #include "base/run_loop.h"
 #include "chrome/browser/sync/profile_sync_service_factory.h"
 #include "chrome/browser/sync/test/integration/apps_helper.h"
+#include "chrome/browser/sync/test/integration/os_sync_test.h"
+#include "chrome/browser/sync/test/integration/profile_sync_service_harness.h"
 #include "chrome/browser/sync/test/integration/status_change_checker.h"
 #include "chrome/browser/sync/test/integration/sync_app_list_helper.h"
 #include "chrome/browser/sync/test/integration/sync_test.h"
@@ -16,9 +18,15 @@
 #include "chrome/browser/ui/app_list/app_list_syncable_service_factory.h"
 #include "chrome/browser/ui/app_list/internal_app/internal_app_metadata.h"
 #include "chrome/browser/ui/app_list/page_break_constants.h"
+#include "components/sync/base/user_selectable_type.h"
 #include "components/sync/driver/sync_service.h"
 #include "components/sync/driver/sync_user_settings.h"
 
+using syncer::UserSelectableOsType;
+using syncer::UserSelectableOsTypeSet;
+using syncer::UserSelectableType;
+using syncer::UserSelectableTypeSet;
+
 namespace {
 
 bool AllProfilesHaveSameAppList() {
@@ -209,3 +217,53 @@ IN_PROC_BROWSER_TEST_F(SingleClientAppListSyncTest, LocalStorage) {
   EXPECT_TRUE(AppListSyncUpdateWaiter(service).Wait());
   EXPECT_TRUE(SyncItemsMatch(service, &compare_service));
 }
+
+// Tests for SplitSettingsSync.
+class SingleClientAppListOsSyncTest : public OsSyncTest {
+ public:
+  SingleClientAppListOsSyncTest() : OsSyncTest(SINGLE_CLIENT) {}
+  ~SingleClientAppListOsSyncTest() override = default;
+};
+
+IN_PROC_BROWSER_TEST_F(SingleClientAppListOsSyncTest,
+                       AppListSyncedForAllAppTypes) {
+  ASSERT_TRUE(SetupSync());
+  syncer::SyncService* service = GetSyncService(0);
+  syncer::SyncUserSettings* settings = service->GetUserSettings();
+
+  // Initially all app types are enabled.
+  ASSERT_TRUE(settings->IsSyncEverythingEnabled());
+  ASSERT_TRUE(settings->IsSyncAllOsTypesEnabled());
+  ASSERT_TRUE(service->GetActiveDataTypes().Has(syncer::APP_LIST));
+  ASSERT_TRUE(service->GetActiveDataTypes().Has(syncer::APPS));
+  ASSERT_TRUE(service->GetActiveDataTypes().Has(syncer::ARC_PACKAGE));
+
+  // Disable all browser types, which disables Chrome apps.
+  settings->SetSelectedTypes(false, UserSelectableTypeSet());
+  GetClient(0)->AwaitSyncSetupCompletion();
+  EXPECT_FALSE(service->GetActiveDataTypes().Has(syncer::APPS));
+  EXPECT_TRUE(service->GetActiveDataTypes().Has(syncer::ARC_PACKAGE));
+
+  // APP_LIST is still synced because ARC apps affect it.
+  EXPECT_TRUE(service->GetActiveDataTypes().Has(syncer::APP_LIST));
+
+  // Enable browser types and disable OS types, which disables ARC apps.
+  settings->SetSelectedTypes(true, UserSelectableTypeSet());
+  settings->SetSelectedOsTypes(false, UserSelectableOsTypeSet());
+  GetClient(0)->AwaitSyncSetupCompletion();
+  EXPECT_TRUE(service->GetActiveDataTypes().Has(syncer::APPS));
+  EXPECT_FALSE(service->GetActiveDataTypes().Has(syncer::ARC_PACKAGE));
+
+  // APP_LIST is still synced because Chrome apps affect it.
+  EXPECT_TRUE(service->GetActiveDataTypes().Has(syncer::APP_LIST));
+
+  // Disable both browser and OS types.
+  settings->SetSelectedTypes(false, UserSelectableTypeSet());
+  settings->SetSelectedOsTypes(false, UserSelectableOsTypeSet());
+  GetClient(0)->AwaitSyncSetupCompletion();
+  EXPECT_FALSE(service->GetActiveDataTypes().Has(syncer::APPS));
+  EXPECT_FALSE(service->GetActiveDataTypes().Has(syncer::ARC_PACKAGE));
+
+  // APP_LIST is not synced.
+  EXPECT_FALSE(service->GetActiveDataTypes().Has(syncer::APP_LIST));
+}

chrome/browser/sync/test/integration/two_client_app_list_sync_test.cc

@@ -11,12 +11,16 @@
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/sync/test/integration/apps_helper.h"
 #include "chrome/browser/sync/test/integration/extensions_helper.h"
+#include "chrome/browser/sync/test/integration/os_sync_test.h"
 #include "chrome/browser/sync/test/integration/profile_sync_service_harness.h"
 #include "chrome/browser/sync/test/integration/sync_app_list_helper.h"
 #include "chrome/browser/sync/test/integration/sync_test.h"
 #include "chrome/browser/sync/test/integration/updated_progress_marker_checker.h"
 #include "chrome/browser/ui/app_list/app_list_syncable_service.h"
 #include "chrome/browser/ui/app_list/app_list_syncable_service_factory.h"
+#include "chromeos/constants/chromeos_features.h"
+#include "components/prefs/pref_service.h"
+#include "components/sync/base/pref_names.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/test/test_utils.h"
 #include "extensions/browser/extension_prefs.h"
@@ -488,8 +492,6 @@ IN_PROC_BROWSER_TEST_P(RemoveDefaultAppSyncTest, Remove) {
 
 INSTANTIATE_TEST_SUITE_P(All, RemoveDefaultAppSyncTest, ::testing::Bool());
 
-#if !defined(OS_MACOSX)
-
 // Install some apps on both clients, then sync. Move an app on one client
 // to a folder and sync. The app lists, including folders, should match.
 IN_PROC_BROWSER_TEST_F(TwoClientAppListSyncTest, MoveToFolder) {
@@ -570,4 +572,48 @@ IN_PROC_BROWSER_TEST_F(TwoClientAppListSyncTest, FolderAddRemove) {
   ASSERT_TRUE(AllProfilesHaveSameAppList());
 }
 
-#endif  // !defined(OS_MACOSX)
+// Tests for SplitSettingsSync.
+class TwoClientAppListOsSyncTest : public TwoClientAppListSyncTest {
+ public:
+  TwoClientAppListOsSyncTest() {
+    settings_feature_list_.InitAndEnableFeature(
+        chromeos::features::kSplitSettingsSync);
+  }
+  ~TwoClientAppListOsSyncTest() override = default;
+
+  // SyncTest
+  bool SetupClients() override {
+    if (!TwoClientAppListSyncTest::SetupClients())
+      return false;
+    // Enable the OS sync feature for all profiles.
+    for (Profile* profile : GetAllProfiles()) {
+      profile->GetPrefs()->SetBoolean(syncer::prefs::kOsSyncFeatureEnabled,
+                                      true);
+    }
+    return true;
+  }
+
+ private:
+  base::test::ScopedFeatureList settings_feature_list_;
+};
+
+IN_PROC_BROWSER_TEST_F(TwoClientAppListOsSyncTest, DisableOsSync) {
+  ASSERT_TRUE(SetupSync());
+  ASSERT_TRUE(AllProfilesHaveSameAppList());
+
+  // Install a Chrome app and sync.
+  InstallApp(GetProfile(0), 0);
+  AwaitQuiescenceAndInstallAppsPendingForSync();
+  ASSERT_TRUE(AllProfilesHaveSameAppList());
+
+  // Disable OS sync on the second client.
+  GetProfile(1)->GetPrefs()->SetBoolean(syncer::prefs::kOsSyncFeatureEnabled,
+                                        false);
+
+  // Install another Chrome app and sync.
+  InstallApp(GetProfile(0), 1);
+  AwaitQuiescenceAndInstallAppsPendingForSync();
+
+  // App list sync still occurs.
+  ASSERT_TRUE(AllProfilesHaveSameAppList());
+}

components/sync/base/user_selectable_type.cc

@@ -72,9 +72,11 @@ UserSelectableTypeInfo GetUserSelectableTypeInfo(UserSelectableType type) {
     case UserSelectableType::kApps: {
       ModelTypeSet model_types = {APPS, APP_SETTINGS, WEB_APPS};
 #if defined(OS_CHROMEOS)
+      // App list must sync if either Chrome apps or ARC apps are synced.
+      model_types.Put(APP_LIST);
       // SplitSettingsSync moves ARC apps under a separate OS setting.
       if (!chromeos::features::IsSplitSettingsSyncEnabled())
-        model_types.PutAll({APP_LIST, ARC_PACKAGE});
+        model_types.Put(ARC_PACKAGE);
 #endif
       return {kAppsTypeName, APPS, model_types};
     }
@@ -100,7 +102,8 @@ UserSelectableTypeInfo GetUserSelectableOsTypeInfo(UserSelectableOsType type) {
   // changed without updating js part.
   switch (type) {
     case UserSelectableOsType::kOsApps:
-      return {"osApps", APP_LIST, {APP_LIST, ARC_PACKAGE}};
+      // App list must sync if either Chrome apps or ARC apps are synced.
+      return {"osApps", ARC_PACKAGE, {ARC_PACKAGE, APP_LIST}};
     case UserSelectableOsType::kOsPreferences:
       return {"osPreferences",
               OS_PREFERENCES,
@@ -155,6 +158,17 @@ UserSelectableType GetUserSelectableTypeFromString(const std::string& type) {
   return UserSelectableType::kLastType;
 }
 
+std::string UserSelectableTypeSetToString(UserSelectableTypeSet types) {
+  std::string result;
+  for (UserSelectableType type : types) {
+    if (!result.empty()) {
+      result += ", ";
+    }
+    result += GetUserSelectableTypeName(type);
+  }
+  return result;
+}
+
 ModelTypeSet UserSelectableTypeToAllModelTypes(UserSelectableType type) {
   return GetUserSelectableTypeInfo(type).model_type_group;
 }

components/sync/base/user_selectable_type.h

@@ -35,6 +35,7 @@ using UserSelectableTypeSet = EnumSet<UserSelectableType,
 
 const char* GetUserSelectableTypeName(UserSelectableType type);
 UserSelectableType GetUserSelectableTypeFromString(const std::string& type);
+std::string UserSelectableTypeSetToString(UserSelectableTypeSet types);
 ModelTypeSet UserSelectableTypeToAllModelTypes(UserSelectableType type);
 
 ModelType UserSelectableTypeToCanonicalModelType(UserSelectableType type);

components/sync/driver/sync_user_settings_impl.cc

@@ -6,6 +6,7 @@
 
 #include "base/metrics/histogram_macros.h"
 #include "components/sync/base/sync_prefs.h"
+#include "components/sync/base/user_selectable_type.h"
 #include "components/sync/driver/sync_service_crypto.h"
 
 #if defined(OS_CHROMEOS)
@@ -101,7 +102,8 @@ UserSelectableTypeSet SyncUserSettingsImpl::GetSelectedTypes() const {
 void SyncUserSettingsImpl::SetSelectedTypes(bool sync_everything,
                                             UserSelectableTypeSet types) {
   UserSelectableTypeSet registered_types = GetRegisteredSelectableTypes();
-  DCHECK(registered_types.HasAll(types));
+  DCHECK(registered_types.HasAll(types))
+      << UserSelectableTypeSetToString(types);
   prefs_->SetSelectedTypes(sync_everything, registered_types, types);
 }