[send-tab-to-self] Make GetEntryPointDisplayReason() a method

No behavior change. Before this CL there was a GetEntryPointDisplayReason(...) free function taking a bunch of services as parameters, including the send-tab-to-self keyed service (SendTabToSelfSyncService). Add SendTabToSelfSyncService::GetEntryPointDisplayReason(const GURL&) and migrate callers. Advantages: - Easier testing, we can fake one service (SendTabToSelfSyncService) instead of many. - Hides dependencies. I might add a new service dependency soon, I don't want to update every caller each time I do so. The free function still exists, moved to an "internal" namespace, and is called by SendTabToSelfSyncService. I would prefer to inline the logic in SendTabToSelfSyncService and unit test that class, but this requires moving the construction of some fields to the parent and it looked weird for one particular WeakPtr. So I decided against. For next CLs we could consider removing "Sync" from the name of the service. Its API isn't tied to sync before this CL and now even less so. Bug: 1468530 Change-Id: I78a50af61d134c28e7b0b146c20e08eb7937e5f7 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4774411 Auto-Submit: Victor Vianna <victorvianna@google.com> Commit-Queue: Victor Vianna <victorvianna@google.com> Reviewed-by: Sergio Collazos <sczs@chromium.org> Cr-Commit-Position: refs/heads/main@{#1184400}
chromium · Aug 16, 2023 · fd4cc6d · fd4cc6d
1 parent 3eebd4d
commit fd4cc6d
Show file tree

Hide file tree

Showing 14 changed files with 164 additions and 168 deletions.
diff --git a/chrome/browser/android/send_tab_to_self/send_tab_to_self_android_bridge.cc b/chrome/browser/android/send_tab_to_self/send_tab_to_self_android_bridge.cc
@@ -15,7 +15,6 @@
 #include "chrome/browser/profiles/profile_android.h"
 #include "chrome/browser/send_tab_to_self/receiving_ui_handler_registry.h"
 #include "chrome/browser/sync/send_tab_to_self_sync_service_factory.h"
-#include "chrome/browser/sync/sync_service_factory.h"
 #include "components/send_tab_to_self/entry_point_display_reason.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"
@@ -133,13 +132,13 @@ JNI_SendTabToSelfAndroidBridge_GetEntryPointDisplayReason(
     JNIEnv* env,
     const JavaParamRef<jobject>& j_profile,
     const JavaParamRef<jstring>& j_url_to_share) {
-  Profile* profile = ProfileAndroid::FromProfileAndroid(j_profile);
+  send_tab_to_self::SendTabToSelfSyncService* service =
+      SendTabToSelfSyncServiceFactory::GetForProfile(
+          ProfileAndroid::FromProfileAndroid(j_profile));
   absl::optional<send_tab_to_self::EntryPointDisplayReason> reason =
-      send_tab_to_self::GetEntryPointDisplayReason(
-          GURL(ConvertJavaStringToUTF8(env, j_url_to_share)),
-          SyncServiceFactory::GetForProfile(profile),
-          SendTabToSelfSyncServiceFactory::GetForProfile(profile),
-          profile->GetPrefs());
+      service ? service->GetEntryPointDisplayReason(
+                    GURL(ConvertJavaStringToUTF8(env, j_url_to_share)))
+              : absl::nullopt;
 
   if (!reason) {
     return nullptr;

diff --git a/chrome/browser/send_tab_to_self/send_tab_to_self_util.cc b/chrome/browser/send_tab_to_self/send_tab_to_self_util.cc
@@ -25,13 +25,12 @@ absl::optional<EntryPointDisplayReason> GetEntryPointDisplayReason(
   if (!web_contents)
     return absl::nullopt;
 
-  auto* profile =
-      Profile::FromBrowserContext(web_contents->GetBrowserContext());
-  return GetEntryPointDisplayReason(
-      web_contents->GetLastCommittedURL(),
-      SyncServiceFactory::GetForProfile(profile),
-      SendTabToSelfSyncServiceFactory::GetForProfile(profile),
-      profile->GetPrefs());
+  send_tab_to_self::SendTabToSelfSyncService* service =
+      SendTabToSelfSyncServiceFactory::GetForProfile(
+          Profile::FromBrowserContext(web_contents->GetBrowserContext()));
+  return service ? service->GetEntryPointDisplayReason(
+                       web_contents->GetLastCommittedURL())
+                 : absl::nullopt;
 }
 
 bool ShouldDisplayEntryPoint(content::WebContents* web_contents) {

diff --git a/chrome/browser/send_tab_to_self/send_tab_to_self_util_unittest.cc b/chrome/browser/send_tab_to_self/send_tab_to_self_util_unittest.cc
@@ -5,19 +5,14 @@
 #include "chrome/browser/send_tab_to_self/send_tab_to_self_util.h"
 
 #include <memory>
-#include <utility>
 
 #include "base/functional/bind.h"
-#include "base/test/scoped_feature_list.h"
 #include "chrome/browser/sync/send_tab_to_self_sync_service_factory.h"
-#include "chrome/browser/sync/sync_service_factory.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/test/base/browser_with_test_window_test.h"
-#include "components/send_tab_to_self/features.h"
 #include "components/send_tab_to_self/send_tab_to_self_sync_service.h"
 #include "components/send_tab_to_self/test_send_tab_to_self_model.h"
-#include "components/sync/test/test_sync_service.h"
 #include "content/public/test/navigation_simulator.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "url/gurl.h"
@@ -29,47 +24,23 @@ namespace {
 const char kHttpsUrl[] = "https://www.foo.com";
 const char kHttpsUrl2[] = "https://www.bar.com";
 
-class FakeSendTabToSelfModel : public TestSendTabToSelfModel {
- public:
-  FakeSendTabToSelfModel() = default;
-  ~FakeSendTabToSelfModel() override = default;
-
-  void SetIsReady(bool is_ready) { is_ready_ = is_ready; }
-  void SetHasValidTargetDevice(bool has_valid_target_device) {
-    if (has_valid_target_device) {
-      DCHECK(is_ready_) << "Target devices are only known if the model's ready";
-    }
-    has_valid_target_device_ = has_valid_target_device;
-  }
-
-  bool IsReady() override { return is_ready_; }
-  bool HasValidTargetDevice() override { return has_valid_target_device_; }
-
- private:
-  bool is_ready_ = false;
-  bool has_valid_target_device_ = false;
-};
-
+// A fake that's always ready to offer send-tab-to-self.
 class FakeSendTabToSelfSyncService : public SendTabToSelfSyncService {
  public:
   FakeSendTabToSelfSyncService() = default;
   ~FakeSendTabToSelfSyncService() override = default;
 
-  FakeSendTabToSelfModel* GetSendTabToSelfModel() override { return &model_; }
-
- private:
-  FakeSendTabToSelfModel model_;
+  absl::optional<EntryPointDisplayReason> GetEntryPointDisplayReason(
+      const GURL&) override {
+    return EntryPointDisplayReason::kOfferFeature;
+  }
 };
 
 std::unique_ptr<KeyedService> BuildFakeSendTabToSelfSyncService(
     content::BrowserContext*) {
   return std::make_unique<FakeSendTabToSelfSyncService>();
 }
 
-std::unique_ptr<KeyedService> BuildTestSyncService(content::BrowserContext*) {
-  return std::make_unique<syncer::TestSyncService>();
-}
-
 class SendTabToSelfUtilTest : public BrowserWithTestWindowTest {
  public:
   void SetUp() override {
@@ -80,35 +51,15 @@ class SendTabToSelfUtilTest : public BrowserWithTestWindowTest {
 
   TestingProfile::TestingFactories GetTestingFactories() override {
     return {{SendTabToSelfSyncServiceFactory::GetInstance(),
-             base::BindRepeating(&BuildFakeSendTabToSelfSyncService)},
-            {SyncServiceFactory::GetInstance(),
-             base::BindRepeating(&BuildTestSyncService)}};
+             base::BindRepeating(&BuildFakeSendTabToSelfSyncService)}};
   }
 
   content::WebContents* web_contents() {
     return browser()->tab_strip_model()->GetActiveWebContents();
   }
-
-  FakeSendTabToSelfSyncService* service() {
-    return static_cast<FakeSendTabToSelfSyncService*>(
-        SendTabToSelfSyncServiceFactory::GetForProfile(profile()));
-  }
-
-  void SignIn() {
-    CoreAccountInfo account;
-    account.gaia = "gaia_id";
-    account.email = "email@test.com";
-    account.account_id = CoreAccountId::FromGaiaId(account.gaia);
-    static_cast<syncer::TestSyncService*>(
-        SyncServiceFactory::GetForProfile(profile()))
-        ->SetAccountInfo(account);
-  }
 };
 
 TEST_F(SendTabToSelfUtilTest, ShouldHideEntryPointInOmniboxWhileNavigating) {
-  SignIn();
-  service()->GetSendTabToSelfModel()->SetIsReady(true);
-  service()->GetSendTabToSelfModel()->SetHasValidTargetDevice(true);
   NavigateAndCommitActiveTab(GURL(kHttpsUrl));
 
   ASSERT_FALSE(web_contents()->IsWaitingForResponse());

diff --git a/chrome/browser/sync/send_tab_to_self_sync_service_factory.cc b/chrome/browser/sync/send_tab_to_self_sync_service_factory.cc
@@ -62,5 +62,5 @@ KeyedService* SendTabToSelfSyncServiceFactory::BuildServiceInstanceFor(
 
   return new send_tab_to_self::SendTabToSelfSyncService(
       chrome::GetChannel(), std::move(store_factory), history_service,
-      device_info_tracker);
+      profile->GetPrefs(), device_info_tracker);
 }
diff --git a/chrome/browser/sync/sync_service_factory.cc b/chrome/browser/sync/sync_service_factory.cc
@@ -50,6 +50,7 @@
 #include "chrome/common/buildflags.h"
 #include "chrome/common/channel_info.h"
 #include "components/network_time/network_time_tracker.h"
+#include "components/send_tab_to_self/send_tab_to_self_sync_service.h"
 #include "components/supervised_user/core/common/buildflags.h"
 #include "components/sync/base/command_line_switches.h"
 #include "components/sync/service/sync_service_impl.h"
@@ -182,6 +183,9 @@ std::unique_ptr<KeyedService> BuildSyncService(
     password_store->OnSyncServiceInitialized(sync_service.get());
   }
 
+  SendTabToSelfSyncServiceFactory::GetForProfile(profile)
+      ->OnSyncServiceInitialized(sync_service.get());
+
   // Allow sync_preferences/ components to use SyncService.
   sync_preferences::PrefServiceSyncable* pref_service =
       PrefServiceSyncableFromProfile(profile);

diff --git a/components/send_tab_to_self/entry_point_display_reason.cc b/components/send_tab_to_self/entry_point_display_reason.cc
@@ -24,7 +24,7 @@ bool ShouldOfferSignin(syncer::SyncService* sync_service,
 #if BUILDFLAG(IS_CHROMEOS_LACROS)
   return false;
 #else
-  return pref_service->GetBoolean(prefs::kSigninAllowed) && sync_service &&
+  return pref_service->GetBoolean(prefs::kSigninAllowed) &&
          sync_service->GetAccountInfo().IsEmpty() &&
          !sync_service->HasDisableReason(
              syncer::SyncService::DISABLE_REASON_ENTERPRISE_POLICY) &&
@@ -34,16 +34,19 @@ bool ShouldOfferSignin(syncer::SyncService* sync_service,
 
 }  // namespace
 
+namespace internal {
+
 absl::optional<EntryPointDisplayReason> GetEntryPointDisplayReason(
     const GURL& url_to_share,
     syncer::SyncService* sync_service,
-    SendTabToSelfSyncService* send_tab_to_self_sync_service,
+    SendTabToSelfModel* send_tab_to_self_model,
     PrefService* pref_service) {
-  if (!url_to_share.SchemeIsHTTPOrHTTPS())
+  if (!url_to_share.SchemeIsHTTPOrHTTPS()) {
     return absl::nullopt;
+  }
 
-  if (!send_tab_to_self_sync_service || !sync_service) {
-    // Can happen in incognito, guest profile, or tests.
+  if (!send_tab_to_self_model || !sync_service) {
+    // Send-tab-to-self can't work properly, don't show the entry point.
     return absl::nullopt;
   }
 
@@ -52,9 +55,7 @@ absl::optional<EntryPointDisplayReason> GetEntryPointDisplayReason(
     return EntryPointDisplayReason::kOfferSignIn;
   }
 
-  SendTabToSelfModel* model =
-      send_tab_to_self_sync_service->GetSendTabToSelfModel();
-  if (!model->IsReady()) {
+  if (!send_tab_to_self_model->IsReady()) {
     syncer::SyncUserSettings* settings = sync_service->GetUserSettings();
     if (sync_service->IsEngineInitialized() &&
         (settings->IsPassphraseRequiredForPreferredDataTypes() ||
@@ -67,7 +68,7 @@ absl::optional<EntryPointDisplayReason> GetEntryPointDisplayReason(
     return absl::nullopt;
   }
 
-  if (!model->HasValidTargetDevice()) {
+  if (!send_tab_to_self_model->HasValidTargetDevice()) {
     return base::FeatureList::IsEnabled(kSendTabToSelfSigninPromo)
                ? absl::make_optional(
                      EntryPointDisplayReason::kInformNoTargetDevice)
@@ -77,4 +78,6 @@ absl::optional<EntryPointDisplayReason> GetEntryPointDisplayReason(
   return EntryPointDisplayReason::kOfferFeature;
 }
 
+}  // namespace internal
+
 }  // namespace send_tab_to_self
diff --git a/components/send_tab_to_self/entry_point_display_reason.h b/components/send_tab_to_self/entry_point_display_reason.h
@@ -16,7 +16,7 @@ class SyncService;
 
 namespace send_tab_to_self {
 
-class SendTabToSelfSyncService;
+class SendTabToSelfModel;
 
 // A Java counterpart will be generated for this enum.
 // GENERATED_JAVA_ENUM_PACKAGE: (
@@ -35,13 +35,20 @@ enum class EntryPointDisplayReason {
   kInformNoTargetDevice,
 };
 
-// |sync_service| and |send_tab_to_self_sync_service| can be null.
+namespace internal {
+
+// Use SendTabToSelfService::GetEntryPointDisplayReason() instead.
+// `sync_service` and `send_tab_to_self_sync_model` can be null and that's
+// handled as if send-tab-to-self or the sync backbone aren't working, so the
+// entry point should be hidden (returns nullopt).
 absl::optional<EntryPointDisplayReason> GetEntryPointDisplayReason(
     const GURL& url_to_share,
     syncer::SyncService* sync_service,
-    SendTabToSelfSyncService* send_tab_to_self_sync_service,
+    SendTabToSelfModel* send_tab_to_self_model,
     PrefService* pref_service);
 
+}  // namespace internal
+
 }  // namespace send_tab_to_self
 
 #endif  // COMPONENTS_SEND_TAB_TO_SELF_ENTRY_POINT_DISPLAY_REASON_H_