Change required datatypes in SharingService according to flags

- This will enable SharingService to register in transport mode once
  1) DeviceInfo is in transport mode (SyncDeviceInfoInTransportMode)
  2) FCM/encryption info in DeviceInfo (SharingUseDeviceInfo)
  3) VAPID key derive from sync (SharingDeriveVapidKey)
- For required sync data types:
  - DEVICE_INFO is always required
  - kSharingUseDeviceInfo disabled -> require PREFERENCES
  - kSharingDeriveVapidKey disabled -> require PREFERENCES

Bug: 1010968
Change-Id: I78aaa6c4ee00098d4c6cbe9ebf9aa922733ed3ca
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1837935
Commit-Queue: Alex Chau <alexchau@chromium.org>
Reviewed-by: Richard Knoll <knollr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#702830}

chrome/browser/sharing/sharing_service.cc

@@ -439,7 +439,7 @@ bool SharingService::IsSyncEnabled() const {
   return sync_service_ &&
          sync_service_->GetTransportState() ==
              syncer::SyncService::TransportState::ACTIVE &&
-         sync_service_->GetActiveDataTypes().Has(syncer::PREFERENCES);
+         sync_service_->GetActiveDataTypes().HasAll(GetRequiredSyncDataTypes());
 }
 
 SharingSyncPreference* SharingService::GetSyncPreferences() const {
@@ -450,11 +450,24 @@ bool SharingService::IsSyncDisabled() const {
   // TODO(alexchau): Better way to make
   // ClickToCallBrowserTest.ContextMenu_DevicesAvailable_SyncTurnedOff pass
   // without unnecessarily checking SyncService::GetDisableReasons.
-  return sync_service_ &&
-         (sync_service_->GetTransportState() ==
-              syncer::SyncService::TransportState::DISABLED ||
-          (sync_service_->GetTransportState() ==
-               syncer::SyncService::TransportState::ACTIVE &&
-           !sync_service_->GetActiveDataTypes().Has(syncer::PREFERENCES)) ||
-          sync_service_->GetDisableReasons());
+  return sync_service_ && (sync_service_->GetTransportState() ==
+                               syncer::SyncService::TransportState::DISABLED ||
+                           (sync_service_->GetTransportState() ==
+                                syncer::SyncService::TransportState::ACTIVE &&
+                            !sync_service_->GetActiveDataTypes().HasAll(
+                                GetRequiredSyncDataTypes())) ||
+                           sync_service_->GetDisableReasons());
+}
+
+syncer::ModelTypeSet SharingService::GetRequiredSyncDataTypes() const {
+  // DeviceInfo is always required to list devices.
+  syncer::ModelTypeSet required_data_types(syncer::DEVICE_INFO);
+
+  // Legacy implementation of device list and VAPID key uses sync preferences.
+  if (!base::FeatureList::IsEnabled(kSharingUseDeviceInfo) ||
+      !base::FeatureList::IsEnabled(kSharingDeriveVapidKey)) {
+    required_data_types.Put(syncer::PREFERENCES);
+  }
+
+  return required_data_types;
 }

chrome/browser/sharing/sharing_service.h

@@ -21,6 +21,7 @@
 #include "chrome/browser/sharing/sharing_send_message_result.h"
 #include "components/gcm_driver/web_push_common.h"
 #include "components/keyed_service/core/keyed_service.h"
+#include "components/sync/base/model_type.h"
 #include "components/sync/driver/sync_service_observer.h"
 #include "components/sync/protocol/device_info_specifics.pb.h"
 #include "components/sync_device_info/device_info_tracker.h"
@@ -156,6 +157,9 @@ class SharingService : public KeyedService,
   // in transitioning state.
   bool IsSyncDisabled() const;
 
+  // Returns all required sync data types to enable Sharing feature.
+  syncer::ModelTypeSet GetRequiredSyncDataTypes() const;
+
   std::unique_ptr<SharingSyncPreference> sync_prefs_;
   std::unique_ptr<VapidKeyManager> vapid_key_manager_;
   std::unique_ptr<SharingDeviceRegistration> sharing_device_registration_;

chrome/browser/sharing/sharing_service_unittest.cc

@@ -455,7 +455,8 @@ TEST_F(SharingServiceTest, DeviceRegistration) {
   scoped_feature_list_.InitAndEnableFeature(kSharingDeviceRegistration);
   test_sync_service_.SetTransportState(
       syncer::SyncService::TransportState::ACTIVE);
-  test_sync_service_.SetActiveDataTypes(syncer::PREFERENCES);
+  test_sync_service_.SetActiveDataTypes(
+      {syncer::DEVICE_INFO, syncer::PREFERENCES});
 
   EXPECT_EQ(SharingService::State::DISABLED, GetSharingService()->GetState());
 
@@ -482,12 +483,50 @@ TEST_F(SharingServiceTest, DeviceRegistration) {
   EXPECT_EQ(SharingService::State::ACTIVE, GetSharingService()->GetState());
 }
 
+TEST_F(SharingServiceTest, DeviceRegistrationPreferenceNotAvailable) {
+  // Enable the feature.
+  scoped_feature_list_.InitAndEnableFeature(kSharingDeviceRegistration);
+  test_sync_service_.SetTransportState(
+      syncer::SyncService::TransportState::ACTIVE);
+  test_sync_service_.SetActiveDataTypes(syncer::DEVICE_INFO);
+
+  EXPECT_EQ(SharingService::State::DISABLED, GetSharingService()->GetState());
+
+  // As sync preferences is not available, registration shouldn't start.
+  EXPECT_CALL(*fcm_handler_, StartListening()).Times(0);
+  test_sync_service_.FireStateChanged();
+  EXPECT_EQ(0, sharing_device_registration_->registration_attempts());
+  EXPECT_EQ(SharingService::State::DISABLED, GetSharingService()->GetState());
+}
+
+TEST_F(SharingServiceTest, DeviceRegistrationTransportMode) {
+  // Enable the registration feature and transport mode required features.
+  scoped_feature_list_.InitWithFeatures(
+      /*enabled_features=*/{kSharingDeviceRegistration, kSharingUseDeviceInfo,
+                            kSharingDeriveVapidKey},
+      /*disabled_features=*/{});
+  test_sync_service_.SetTransportState(
+      syncer::SyncService::TransportState::ACTIVE);
+  test_sync_service_.SetActiveDataTypes(syncer::DEVICE_INFO);
+
+  EXPECT_EQ(SharingService::State::DISABLED, GetSharingService()->GetState());
+
+  // Expect registration to be successful on sync state changed.
+  sharing_device_registration_->SetResult(
+      SharingDeviceRegistrationResult::kSuccess);
+  EXPECT_CALL(*fcm_handler_, StartListening()).Times(1);
+  test_sync_service_.FireStateChanged();
+  EXPECT_EQ(1, sharing_device_registration_->registration_attempts());
+  EXPECT_EQ(SharingService::State::ACTIVE, GetSharingService()->GetState());
+}
+
 TEST_F(SharingServiceTest, DeviceRegistrationTransientError) {
   // Enable the feature.
   scoped_feature_list_.InitAndEnableFeature(kSharingDeviceRegistration);
   test_sync_service_.SetTransportState(
       syncer::SyncService::TransportState::ACTIVE);
-  test_sync_service_.SetActiveDataTypes(syncer::PREFERENCES);
+  test_sync_service_.SetActiveDataTypes(
+      {syncer::DEVICE_INFO, syncer::PREFERENCES});
 
   EXPECT_EQ(SharingService::State::DISABLED, GetSharingService()->GetState());
 
@@ -545,7 +584,8 @@ TEST_F(SharingServiceTest, DeviceRegisterAndUnregister) {
   scoped_feature_list_.InitAndEnableFeature(kSharingDeviceRegistration);
   test_sync_service_.SetTransportState(
       syncer::SyncService::TransportState::ACTIVE);
-  test_sync_service_.SetActiveDataTypes(syncer::PREFERENCES);
+  test_sync_service_.SetActiveDataTypes(
+      {syncer::DEVICE_INFO, syncer::PREFERENCES});
 
   // Create new SharingService instance with feature enabled at constructor.
   GetSharingService();
@@ -601,7 +641,7 @@ TEST_F(SharingServiceTest, DeviceRegisterAndUnregister) {
   EXPECT_EQ(SharingService::State::ACTIVE, GetSharingService()->GetState());
 
   // Disable syncing of preference and un-registration should happen.
-  test_sync_service_.SetActiveDataTypes(syncer::ModelTypeSet());
+  test_sync_service_.SetActiveDataTypes(syncer::DEVICE_INFO);
   EXPECT_CALL(*fcm_handler_, StopListening()).Times(1);
   test_sync_service_.FireStateChanged();
   EXPECT_EQ(2, sharing_device_registration_->registration_attempts());
@@ -613,7 +653,8 @@ TEST_F(SharingServiceTest, StartListeningToFCMAtConstructor) {
   scoped_feature_list_.InitAndEnableFeature(kSharingDeviceRegistration);
   test_sync_service_.SetTransportState(
       syncer::SyncService::TransportState::ACTIVE);
-  test_sync_service_.SetActiveDataTypes(syncer::PREFERENCES);
+  test_sync_service_.SetActiveDataTypes(
+      {syncer::DEVICE_INFO, syncer::PREFERENCES});
 
   // Create new SharingService instance with FCM already registered at
   // constructor.