Clean up SharingService

- Removed unused methods
- Add ForTest for testing methods
- Unified SharingMessageHandler creation

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

chrome/browser/sharing/click_to_call/click_to_call_utils_unittest.cc

@@ -40,7 +40,7 @@ const char kSelectionTextWithNumber[] = "9876543210";
 
 class MockSharingDeviceRegistration : public SharingDeviceRegistration {
  public:
-  explicit MockSharingDeviceRegistration()
+  MockSharingDeviceRegistration()
       : SharingDeviceRegistration(/* pref_service_= */ nullptr,
                                   /* sharing_sync_preference_= */ nullptr,
                                   /* instance_id_driver_= */ nullptr,
@@ -54,28 +54,6 @@ class MockSharingDeviceRegistration : public SharingDeviceRegistration {
   DISALLOW_COPY_AND_ASSIGN(MockSharingDeviceRegistration);
 };
 
-class MockSharingService : public SharingService {
- public:
-  explicit MockSharingService(std::unique_ptr<SharingFCMHandler> fcm_handler)
-      : SharingService(/* sync_prefs= */ nullptr,
-                       /* vapid_key_manager= */ nullptr,
-                       std::make_unique<MockSharingDeviceRegistration>(),
-                       /* fcm_sender= */ nullptr,
-                       std::move(fcm_handler),
-                       /* gcm_driver= */ nullptr,
-                       /* device_info_tracker= */ nullptr,
-                       /* local_device_info_provider= */ nullptr,
-                       /* sync_service */ nullptr,
-                       /* notification_display_service= */ nullptr) {}
-
-  ~MockSharingService() override = default;
-
-  MOCK_CONST_METHOD0(GetState, State());
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(MockSharingService);
-};
-
 class ClickToCallUtilsTest : public testing::Test {
  public:
   ClickToCallUtilsTest() = default;
@@ -106,8 +84,17 @@ class ClickToCallUtilsTest : public testing::Test {
     if (!create_service_)
       return nullptr;
 
-    return std::make_unique<NiceMock<MockSharingService>>(
-        std::make_unique<SharingFCMHandler>(nullptr, nullptr, nullptr));
+    return std::make_unique<SharingService>(
+        /* sync_prefs= */ nullptr,
+        /* vapid_key_manager= */ nullptr,
+        std::make_unique<MockSharingDeviceRegistration>(),
+        /* fcm_sender= */ nullptr,
+        std::make_unique<SharingFCMHandler>(nullptr, nullptr, nullptr),
+        /* gcm_driver= */ nullptr,
+        /* device_info_tracker= */ nullptr,
+        /* local_device_info_provider= */ nullptr,
+        /* sync_service */ nullptr,
+        /* notification_display_service= */ nullptr);
   }
 
   base::test::ScopedFeatureList scoped_feature_list_;

chrome/browser/sharing/shared_clipboard/shared_clipboard_utils_unittest.cc

@@ -36,7 +36,7 @@ const char kText[] = "Some text to copy to phone device.";
 
 class MockSharingDeviceRegistration : public SharingDeviceRegistration {
  public:
-  explicit MockSharingDeviceRegistration()
+  MockSharingDeviceRegistration()
       : SharingDeviceRegistration(/* pref_service_= */ nullptr,
                                   /* sharing_sync_preference_= */ nullptr,
                                   /* instance_id_driver_= */ nullptr,
@@ -50,28 +50,6 @@ class MockSharingDeviceRegistration : public SharingDeviceRegistration {
   DISALLOW_COPY_AND_ASSIGN(MockSharingDeviceRegistration);
 };
 
-class MockSharingService : public SharingService {
- public:
-  explicit MockSharingService(std::unique_ptr<SharingFCMHandler> fcm_handler)
-      : SharingService(/* sync_prefs= */ nullptr,
-                       /* vapid_key_manager= */ nullptr,
-                       std::make_unique<MockSharingDeviceRegistration>(),
-                       /* fcm_sender= */ nullptr,
-                       std::move(fcm_handler),
-                       /* gcm_driver= */ nullptr,
-                       /* device_info_tracker= */ nullptr,
-                       /* local_device_info_provider= */ nullptr,
-                       /* sync_service */ nullptr,
-                       /* notification_display_service= */ nullptr) {}
-
-  ~MockSharingService() override = default;
-
-  MOCK_CONST_METHOD0(GetState, State());
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(MockSharingService);
-};
-
 class SharedClipboardUtilsTest : public testing::Test {
  public:
   SharedClipboardUtilsTest() = default;
@@ -90,8 +68,17 @@ class SharedClipboardUtilsTest : public testing::Test {
     if (!create_service_)
       return nullptr;
 
-    return std::make_unique<NiceMock<MockSharingService>>(
-        std::make_unique<SharingFCMHandler>(nullptr, nullptr, nullptr));
+    return std::make_unique<SharingService>(
+        /* sync_prefs= */ nullptr,
+        /* vapid_key_manager= */ nullptr,
+        std::make_unique<MockSharingDeviceRegistration>(),
+        /* fcm_sender= */ nullptr,
+        std::make_unique<SharingFCMHandler>(nullptr, nullptr, nullptr),
+        /* gcm_driver= */ nullptr,
+        /* device_info_tracker= */ nullptr,
+        /* local_device_info_provider= */ nullptr,
+        /* sync_service */ nullptr,
+        /* notification_display_service= */ nullptr);
   }
 
   base::test::ScopedFeatureList scoped_feature_list_;

chrome/browser/sharing/sharing_browsertest.cc

@@ -147,7 +147,8 @@ std::unique_ptr<TestRenderViewContextMenu> SharingBrowserTest::InitContextMenu(
 void SharingBrowserTest::CheckLastReceiver(
     const std::string& device_guid) const {
   auto sharing_info =
-      sharing_service_->GetSyncPreferences()->GetSharingInfo(device_guid);
+      sharing_service_->GetSyncPreferencesForTesting()->GetSharingInfo(
+          device_guid);
   ASSERT_TRUE(sharing_info);
   EXPECT_EQ(sharing_info->fcm_token, gcm_service_->last_receiver_id());
 }

chrome/browser/sharing/sharing_service.cc

@@ -37,6 +37,13 @@
 #include "content/public/browser/browser_task_traits.h"
 #include "ui/base/l10n/l10n_util.h"
 
+#if defined(OS_ANDROID)
+#include "chrome/browser/sharing/shared_clipboard/shared_clipboard_message_handler_android.h"
+#include "chrome/browser/sharing/sharing_service_proxy_android.h"
+#else
+#include "chrome/browser/sharing/shared_clipboard/shared_clipboard_message_handler_desktop.h"
+#endif  // defined(OS_ANDROID)
+
 namespace {
 // Util function to return a string denoting the type of device.
 std::string GetDeviceType(sync_pb::SyncEnums::DeviceType type) {
@@ -160,29 +167,33 @@ SharingService::SharingService(
   }
 
   // Initialize sharing handlers.
+  fcm_handler_->AddSharingHandler(
+      chrome_browser_sharing::SharingMessage::kPingMessage,
+      &ping_message_handler_);
+
   fcm_handler_->AddSharingHandler(
       chrome_browser_sharing::SharingMessage::kAckMessage,
       &ack_message_handler_);
   ack_message_handler_.AddObserver(this);
-  fcm_handler_->AddSharingHandler(
-      chrome_browser_sharing::SharingMessage::kPingMessage,
-      &ping_message_handler_);
+
 #if defined(OS_ANDROID)
+  // Note: IsClickToCallSupported() is not used as it requires JNI call.
   if (base::FeatureList::IsEnabled(kClickToCallReceiver)) {
     fcm_handler_->AddSharingHandler(
         chrome_browser_sharing::SharingMessage::kClickToCallMessage,
-        sharing_service_proxy_android_.click_to_call_message_handler());
+        &click_to_call_message_handler_);
   }
-
-  shared_clipboard_message_handler_ =
-      std::make_unique<SharedClipboardMessageHandlerAndroid>(this);
-#else
-  shared_clipboard_message_handler_ =
-      std::make_unique<SharedClipboardMessageHandlerDesktop>(
-          this, notification_display_service);
 #endif  // defined(OS_ANDROID)
 
   if (sharing_device_registration_->IsSharedClipboardSupported()) {
+#if defined(OS_ANDROID)
+    shared_clipboard_message_handler_ =
+        std::make_unique<SharedClipboardMessageHandlerAndroid>(this);
+#else
+    shared_clipboard_message_handler_ =
+        std::make_unique<SharedClipboardMessageHandlerDesktop>(
+            this, notification_display_service);
+#endif  // defined(OS_ANDROID)
     fcm_handler_->AddSharingHandler(
         chrome_browser_sharing::SharingMessage::kSharedClipboardMessage,
         shared_clipboard_message_handler_.get());
@@ -325,6 +336,14 @@ void SharingService::SetDeviceInfoTrackerForTesting(
   device_info_tracker_ = tracker;
 }
 
+SharingService::State SharingService::GetStateForTesting() const {
+  return state_;
+}
+
+SharingSyncPreference* SharingService::GetSyncPreferencesForTesting() const {
+  return sync_prefs_.get();
+}
+
 void SharingService::OnMessageSent(
     base::TimeTicks start_time,
     const std::string& message_guid,
@@ -390,14 +409,6 @@ void SharingService::OnDeviceInfoChange() {
   device_candidates_initialized_callbacks_.clear();
 }
 
-void SharingService::RegisterHandler(
-    chrome_browser_sharing::SharingMessage::PayloadCase payload_type,
-    SharingMessageHandler* handler) {}
-
-SharingService::State SharingService::GetState() const {
-  return state_;
-}
-
 void SharingService::OnSyncShutdown(syncer::SyncService* sync) {
   if (sync_service_ && sync_service_->HasObserver(this))
     sync_service_->RemoveObserver(this);
@@ -549,10 +560,6 @@ bool SharingService::IsSyncEnabled() const {
          sync_service_->GetActiveDataTypes().HasAll(GetRequiredSyncDataTypes());
 }
 
-SharingSyncPreference* SharingService::GetSyncPreferences() const {
-  return sync_prefs_.get();
-}
-
 bool SharingService::IsSyncDisabled() const {
   return sync_service_ && (sync_service_->GetTransportState() ==
                                syncer::SyncService::TransportState::DISABLED ||

chrome/browser/sharing/sharing_service.h

@@ -29,10 +29,8 @@
 #include "net/base/backoff_entry.h"
 
 #if defined(OS_ANDROID)
-#include "chrome/browser/sharing/shared_clipboard/shared_clipboard_message_handler_android.h"
+#include "chrome/browser/sharing/click_to_call/click_to_call_message_handler_android.h"
 #include "chrome/browser/sharing/sharing_service_proxy_android.h"
-#else
-#include "chrome/browser/sharing/shared_clipboard/shared_clipboard_message_handler_desktop.h"
 #endif  // defined(OS_ANDROID)
 
 namespace gcm {
@@ -45,9 +43,9 @@ class SyncService;
 }  // namespace syncer
 
 class NotificationDisplayService;
+class SharedClipboardMessageHandler;
 class SharingFCMHandler;
 class SharingFCMSender;
-class SharingMessageHandler;
 class SharingSyncPreference;
 class VapidKeyManager;
 enum class SharingDeviceRegistrationResult;
@@ -109,24 +107,20 @@ class SharingService : public KeyedService,
       chrome_browser_sharing::SharingMessage message,
       SendMessageCallback callback);
 
-  // Registers a handler of a given SharingMessage payload type.
-  void RegisterHandler(
-      chrome_browser_sharing::SharingMessage::PayloadCase payload_type,
-      SharingMessageHandler* handler);
-
-  // Returns the current state of SharingService.
-  virtual State GetState() const;
-
   // Used to register devices with required capabilities in tests.
   void RegisterDeviceInTesting(
       std::set<sync_pb::SharingSpecificFields_EnabledFeatures> enabled_features,
       SharingDeviceRegistration::RegistrationCallback callback);
 
-  SharingSyncPreference* GetSyncPreferences() const;
-
   // Used to fake client names in integration tests.
   void SetDeviceInfoTrackerForTesting(syncer::DeviceInfoTracker* tracker);
 
+  // Returns the current state of SharingService for testing.
+  State GetStateForTesting() const;
+
+  // Returns SharingSyncPreference for integration tests.
+  SharingSyncPreference* GetSyncPreferencesForTesting() const;
+
  private:
   // Overrides for syncer::SyncServiceObserver.
   void OnSyncShutdown(syncer::SyncService* sync) override;
@@ -141,7 +135,6 @@ class SharingService : public KeyedService,
   void OnDeviceInfoChange() override;
 
   void RefreshVapidKey();
-
   void RegisterDevice();
   void UnregisterDevice();
 
@@ -188,24 +181,23 @@ class SharingService : public KeyedService,
   std::unique_ptr<SharingDeviceRegistration> sharing_device_registration_;
   std::unique_ptr<SharingFCMSender> fcm_sender_;
   std::unique_ptr<SharingFCMHandler> fcm_handler_;
+
   syncer::DeviceInfoTracker* device_info_tracker_;
   syncer::LocalDeviceInfoProvider* local_device_info_provider_;
   syncer::SyncService* sync_service_;
-  AckMessageHandler ack_message_handler_;
-  PingMessageHandler ping_message_handler_;
+
   net::BackoffEntry backoff_entry_;
   State state_;
-  std::vector<base::OnceClosure> device_candidates_initialized_callbacks_;
   bool is_observing_device_info_tracker_;
   std::unique_ptr<syncer::LocalDeviceInfoProvider::Subscription>
       local_device_info_ready_subscription_;
 
+  // List of callbacks for AddDeviceCandidatesInitializedObserver.
+  std::vector<base::OnceClosure> device_candidates_initialized_callbacks_;
   // Map of random GUID to SendMessageCallback.
   std::map<std::string, SendMessageCallback> send_message_callbacks_;
-
   // Map of FCM message_id to time at start of send message request to FCM.
   std::map<std::string, base::TimeTicks> send_message_times_;
-
   // Map of FCM message_id to random GUID.
   std::map<std::string, std::string> message_guids_;
 
@@ -213,6 +205,11 @@ class SharingService : public KeyedService,
   SharingServiceProxyAndroid sharing_service_proxy_android_{this};
 #endif  // defined(OS_ANDROID)
 
+  PingMessageHandler ping_message_handler_;
+  AckMessageHandler ack_message_handler_;
+#if defined(OS_ANDROID)
+  ClickToCallMessageHandler click_to_call_message_handler_;
+#endif  // defined(OS_ANDROID)
   std::unique_ptr<SharedClipboardMessageHandler>
       shared_clipboard_message_handler_;
 

chrome/browser/sharing/sharing_service_proxy_android.h

@@ -7,8 +7,6 @@
 
 #include "base/android/jni_android.h"
 #include "base/macros.h"
-#include "chrome/browser/sharing/click_to_call/click_to_call_message_handler_android.h"
-#include "chrome/browser/sharing/sharing_message_handler.h"
 
 class SharingService;
 
@@ -18,10 +16,6 @@ class SharingServiceProxyAndroid {
   explicit SharingServiceProxyAndroid(SharingService* sharing_service);
   ~SharingServiceProxyAndroid();
 
-  ClickToCallMessageHandler* click_to_call_message_handler() {
-    return &click_to_call_message_handler_;
-  }
-
   void SendSharedClipboardMessage(
       JNIEnv* env,
       const base::android::JavaParamRef<jstring>& j_guid,
@@ -39,7 +33,6 @@ class SharingServiceProxyAndroid {
 
  private:
   SharingService* sharing_service_ = nullptr;
-  ClickToCallMessageHandler click_to_call_message_handler_;
 
   DISALLOW_COPY_AND_ASSIGN(SharingServiceProxyAndroid);
 };