[butter/self-share] Allow client to receive tab in sync transport mode

If the kSendTabToSelfWhenSignedIn feature is enabled,
IsReceivingEnabledByUserOnThisDevice() will now return true both for
signed-out and signed-in but not syncing users. This WAI since this
information will never be sent to other clients unless the user is
actually signed-in and sync-the-transport is running.

It's hard to use to the SyncService API in this layer due to cyclic
dependencies between DeviceInfoSyncService and ProfileSyncService. So
the code still relies on the state of SyncPrefs, particularly
IsFirstSetupComplete and SyncRequested.

Bug: 956722
Change-Id: I60318e730a250c48d73417b71a2f23b263d665ee
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2526426Reviewed-by: sebsg <sebsg@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Commit-Queue: Victor Vianna <victorvianna@google.com>
Cr-Commit-Position: refs/heads/master@{#830204}

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

@@ -2,15 +2,19 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "base/callback_list.h"
 #include "base/run_loop.h"
+#include "build/build_config.h"
 #include "chrome/browser/history/history_service_factory.h"
 #include "chrome/browser/send_tab_to_self/send_tab_to_self_util.h"
 #include "chrome/browser/sync/device_info_sync_service_factory.h"
 #include "chrome/browser/sync/send_tab_to_self_sync_service_factory.h"
 #include "chrome/browser/sync/test/integration/profile_sync_service_harness.h"
+#include "chrome/browser/sync/test/integration/secondary_account_helper.h"
 #include "chrome/browser/sync/test/integration/send_tab_to_self_helper.h"
 #include "chrome/browser/sync/test/integration/sync_test.h"
 #include "components/history/core/browser/history_service.h"
+#include "components/send_tab_to_self/features.h"
 #include "components/send_tab_to_self/send_tab_to_self_bridge.h"
 #include "components/send_tab_to_self/send_tab_to_self_model.h"
 #include "components/send_tab_to_self/send_tab_to_self_sync_service.h"
@@ -19,6 +23,7 @@
 #include "components/sync_device_info/device_info_sync_service.h"
 #include "components/sync_device_info/local_device_info_provider.h"
 #include "content/public/test/browser_test.h"
+#include "services/network/test/test_url_loader_factory.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "url/gurl.h"
 
@@ -29,8 +34,6 @@ class TwoClientSendTabToSelfSyncTest : public SyncTest {
   ~TwoClientSendTabToSelfSyncTest() override {}
 
  private:
-  base::test::ScopedFeatureList scoped_list_;
-
   DISALLOW_COPY_AND_ASSIGN(TwoClientSendTabToSelfSyncTest);
 };
 
@@ -267,3 +270,62 @@ IN_PROC_BROWSER_TEST_F(TwoClientSendTabToSelfSyncTest,
       send_tab_to_self_helper::SendTabToSelfUrlOpenedChecker(service0, kUrl)
           .Wait());
 }
+
+class TwoClientSendTabToSelfSyncTestWithSendTabToSelfWhenSignedIn
+    : public TwoClientSendTabToSelfSyncTest {
+ public:
+  TwoClientSendTabToSelfSyncTestWithSendTabToSelfWhenSignedIn() {
+    feature_list_.InitAndEnableFeature(
+        send_tab_to_self::kSendTabToSelfWhenSignedIn);
+  }
+  ~TwoClientSendTabToSelfSyncTestWithSendTabToSelfWhenSignedIn() override =
+      default;
+
+  void SetUpInProcessBrowserTestFixture() override {
+    TwoClientSendTabToSelfSyncTest::SetUpInProcessBrowserTestFixture();
+    test_signin_client_subscription_ =
+        secondary_account_helper::SetUpSigninClient(&test_url_loader_factory_);
+  }
+
+ protected:
+  network::TestURLLoaderFactory test_url_loader_factory_;
+
+ private:
+  base::test::ScopedFeatureList feature_list_;
+  base::CallbackListSubscription test_signin_client_subscription_;
+};
+
+// Non-primary accounts don't exist on ChromeOS.
+#if !defined(OS_CHROMEOS)
+IN_PROC_BROWSER_TEST_F(
+    TwoClientSendTabToSelfSyncTestWithSendTabToSelfWhenSignedIn,
+    SignedInClientCanReceive) {
+  ASSERT_TRUE(SetupClients()) << "SetupClients() failed.";
+
+  // Set up one client syncing and the other signed-in but not syncing.
+  GetClient(0)->SetupSync();
+  secondary_account_helper::SignInSecondaryAccount(
+      GetProfile(1), &test_url_loader_factory_, "user@g.com");
+  GetClient(1)->AwaitSyncTransportActive();
+
+  DeviceInfoSyncServiceFactory::GetForProfile(GetProfile(1))
+      ->GetDeviceInfoTracker()
+      ->ForcePulseForTest();
+  DeviceInfoSyncServiceFactory::GetForProfile(GetProfile(0))
+      ->GetDeviceInfoTracker()
+      ->ForcePulseForTest();
+
+  ASSERT_TRUE(send_tab_to_self_helper::SendTabToSelfMultiDeviceActiveChecker(
+                  DeviceInfoSyncServiceFactory::GetForProfile(GetProfile(1))
+                      ->GetDeviceInfoTracker())
+                  .Wait());
+
+  std::vector<std::unique_ptr<syncer::DeviceInfo>> device_infos =
+      DeviceInfoSyncServiceFactory::GetForProfile(GetProfile(1))
+          ->GetDeviceInfoTracker()
+          ->GetAllDeviceInfo();
+  ASSERT_EQ(2u, device_infos.size());
+  EXPECT_TRUE(device_infos[0]->send_tab_to_self_receiving_enabled());
+  EXPECT_TRUE(device_infos[1]->send_tab_to_self_receiving_enabled());
+}
+#endif  // !defined(OS_CHROMEOS)

components/send_tab_to_self/features.cc

@@ -4,6 +4,7 @@
 
 #include "components/send_tab_to_self/features.h"
 
+#include "base/feature_list.h"
 #include "components/sync/base/sync_prefs.h"
 #include "components/sync/base/user_selectable_type.h"
 
@@ -19,12 +20,24 @@ bool IsReceivingEnabledByUserOnThisDevice(PrefService* prefs) {
   // TODO(crbug.com/1015322): SyncPrefs is used directly instead of methods in
   // SyncService due to a dependency of ProfileSyncService on
   // DeviceInfoSyncService. IsReceivingEnabledByUserOnThisDevice is ultimately
-  // used by DeviceInfoSyncClient which is owend by DeviceInfoSyncService.
+  // used by DeviceInfoSyncClient which is owned by DeviceInfoSyncService.
   syncer::SyncPrefs sync_prefs(prefs);
-  // As per documentation in SyncUserSettings, IsSyncRequested indicates user
-  // wants Sync to run, when combined with IsFirstSetupComplete, indicates
-  // whether user has consented to Sync.
-  return sync_prefs.IsSyncRequested() && sync_prefs.IsFirstSetupComplete() &&
-         sync_prefs.GetSelectedTypes().Has(syncer::UserSelectableType::kTabs);
+
+  if (sync_prefs.IsFirstSetupComplete()) {
+    // Sync-the-feature was fully setup. Regardless of
+    // kSendTabToSelfWhenSignedIn, the user-configurable bits should be
+    // respected in this state.
+    return sync_prefs.IsSyncRequested() &&
+           sync_prefs.GetSelectedTypes().Has(syncer::UserSelectableType::kTabs);
+  }
+
+  // Sync-the-feature is disabled or the consent is missing (e.g. sync setup in
+  // progress). If kSendTabToSelfWhenSignedIn is disabled, receiving shouldn't
+  // be allowed in this state. If kSendTabToSelfWhenSignedIn is enabled, the
+  // method can return true without actually checking whether the user is
+  // signed-in: if they are not, sync-the-transport won't run and receiving tabs
+  // would be impossible anyway.
+  return base::FeatureList::IsEnabled(kSendTabToSelfWhenSignedIn);
 }
+
 }  // namespace send_tab_to_self

components/send_tab_to_self/features.h

@@ -21,10 +21,10 @@ extern const base::Feature kSendTabToSelfOmniboxSendingAnimation;
 extern const base::Feature kSendTabToSelfWhenSignedIn;
 
 // Returns whether the receiving components of the feature is enabled on this
-// device. This is different from IsReceivingEnabled in SendTabToSelfUtil
-// because it doesn't rely on the SendTabToSelfSyncService to be actively up and
-// ready.
+// device. This doesn't rely on the SendTabToSelfSyncService to be actively up
+// and ready.
 bool IsReceivingEnabledByUserOnThisDevice(PrefService* prefs);
+
 }  // namespace send_tab_to_self
 
 #endif  // COMPONENTS_SEND_TAB_TO_SELF_FEATURES_H_

components/send_tab_to_self/features_unittest.cc

@@ -7,6 +7,7 @@
 #include <memory>
 #include "base/test/task_environment.h"
 
+#include "base/test/scoped_feature_list.h"
 #include "components/sync/base/sync_prefs.h"
 #include "components/sync_preferences/testing_pref_service_syncable.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -26,10 +27,10 @@ class SendTabToSelfFeaturesTest : public testing::Test {
   sync_preferences::TestingPrefServiceSyncable prefs_;
   std::unique_ptr<syncer::SyncPrefs> sync_prefs_;
   base::test::TaskEnvironment task_environment_;
+  base::test::ScopedFeatureList features_;
 };
 
-TEST_F(SendTabToSelfFeaturesTest,
-       IsReceivingEnabledByUserOnThisDevice_Enabled) {
+TEST_F(SendTabToSelfFeaturesTest, ReceivingEnabledIfSyncTheFeatureEnabled) {
   sync_prefs_->SetSyncRequested(true);
   sync_prefs_->SetFirstSetupComplete();
   sync_prefs_->SetSelectedTypes(
@@ -41,7 +42,7 @@ TEST_F(SendTabToSelfFeaturesTest,
 }
 
 TEST_F(SendTabToSelfFeaturesTest,
-       IsReceivingEnabledByUserOnThisDevice_SyncNotRequested) {
+       ReceivingDisabledIfSyncTheFeatureEnabledButStopped) {
   sync_prefs_->SetSyncRequested(false);
   sync_prefs_->SetFirstSetupComplete();
   sync_prefs_->SetSelectedTypes(
@@ -53,7 +54,32 @@ TEST_F(SendTabToSelfFeaturesTest,
 }
 
 TEST_F(SendTabToSelfFeaturesTest,
-       IsReceivingEnabledByUserOnThisDevice_FirstSetupNotCompleted) {
+       ReceivingDisabledIfSyncTheFeatureEnabledButTabsNotSelected) {
+  sync_prefs_->SetSyncRequested(true);
+  sync_prefs_->SetFirstSetupComplete();
+  sync_prefs_->SetSelectedTypes(
+      /*keep_everything_synced=*/false,
+      /*registered_types=*/syncer::UserSelectableTypeSet::All(),
+      /*selected_types=*/{});
+
+  EXPECT_FALSE(IsReceivingEnabledByUserOnThisDevice(&prefs_));
+}
+
+TEST_F(SendTabToSelfFeaturesTest, ReceivingDisabledIfSyncTheFeatureDisabled) {
+  features_.InitAndDisableFeature(kSendTabToSelfWhenSignedIn);
+  sync_prefs_->SetSelectedTypes(
+      /*keep_everything_synced=*/false,
+      /*registered_types=*/syncer::UserSelectableTypeSet::All(),
+      /*selected_types=*/{});
+
+  EXPECT_FALSE(IsReceivingEnabledByUserOnThisDevice(&prefs_));
+}
+
+TEST_F(
+    SendTabToSelfFeaturesTest,
+    ReceivingDisabledIfSyncSetupIncomplete__SendTabToSelfWhenSignedInEnabled) {
+  features_.InitAndDisableFeature(kSendTabToSelfWhenSignedIn);
+
   sync_prefs_->SetSyncRequested(true);
   // Skip setting FirstSetupComplete.
   sync_prefs_->SetSelectedTypes(
@@ -61,19 +87,36 @@ TEST_F(SendTabToSelfFeaturesTest,
       /*registered_types=*/syncer::UserSelectableTypeSet::All(),
       /*selected_types=*/{syncer::UserSelectableType::kTabs});
 
+  // Should wait for the setup to complete.
   EXPECT_FALSE(IsReceivingEnabledByUserOnThisDevice(&prefs_));
 }
 
 TEST_F(SendTabToSelfFeaturesTest,
-       IsReceivingEnabledByUserOnThisDevice_TabsNotSelected) {
+       ReceivingEnabledIfSyncSetupIncomplete_SendTabToSelfWhenSignedInEnabled) {
+  features_.InitAndEnableFeature(kSendTabToSelfWhenSignedIn);
+
   sync_prefs_->SetSyncRequested(true);
-  sync_prefs_->SetFirstSetupComplete();
+  // Skip setting FirstSetupComplete.
+  sync_prefs_->SetSelectedTypes(
+      /*keep_everything_synced=*/false,
+      /*registered_types=*/syncer::UserSelectableTypeSet::All(),
+      /*selected_types=*/{syncer::UserSelectableType::kTabs});
+
+  // While the setup isn't complete, the client is still treated as having
+  // sync-the-feature disabled.
+  EXPECT_TRUE(IsReceivingEnabledByUserOnThisDevice(&prefs_));
+}
+
+TEST_F(
+    SendTabToSelfFeaturesTest,
+    ReceivingEnabledIfSyncTheFeatureDisabled_SendTabToSelfWhenSignedInEnabled) {
+  features_.InitAndEnableFeature(kSendTabToSelfWhenSignedIn);
   sync_prefs_->SetSelectedTypes(
       /*keep_everything_synced=*/false,
       /*registered_types=*/syncer::UserSelectableTypeSet::All(),
       /*selected_types=*/{});
 
-  EXPECT_FALSE(IsReceivingEnabledByUserOnThisDevice(&prefs_));
+  EXPECT_TRUE(IsReceivingEnabledByUserOnThisDevice(&prefs_));
 }
 
 }  // namespace