Instant Tethering: Remove unused BleAdvertiserImpl.

R=hansberry@chromium.org

Bug: 903991
Test: none
Change-Id: I9445ee9d89a9381779dcbe1c0ffea703d9836cfb
Reviewed-on: https://chromium-review.googlesource.com/c/1343041
Commit-Queue: James Hawkins <jhawkins@chromium.org>
Reviewed-by: Ryan Hansberry <hansberry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#609588}

chromeos/components/tether/BUILD.gn

@@ -17,8 +17,6 @@ static_library("tether") {
     "ble_advertisement_device_queue.h",
     "ble_advertiser.cc",
     "ble_advertiser.h",
-    "ble_advertiser_impl.cc",
-    "ble_advertiser_impl.h",
     "ble_connection_manager.cc",
     "ble_connection_manager.h",
     "ble_connection_metrics_logger.cc",
@@ -250,7 +248,6 @@ source_set("unit_tests") {
     "active_host_unittest.cc",
     "asynchronous_shutdown_object_container_impl_unittest.cc",
     "ble_advertisement_device_queue_unittest.cc",
-    "ble_advertiser_impl_unittest.cc",
     "ble_connection_manager_unittest.cc",
     "ble_connection_metrics_logger_unittest.cc",
     "ble_scanner_impl_unittest.cc",

chromeos/components/tether/asynchronous_shutdown_object_container_impl.cc

@@ -7,7 +7,6 @@
 #include "base/memory/ptr_util.h"
 #include "chromeos/chromeos_features.h"
 #include "chromeos/components/tether/ble_advertisement_device_queue.h"
-#include "chromeos/components/tether/ble_advertiser_impl.h"
 #include "chromeos/components/tether/ble_connection_metrics_logger.h"
 #include "chromeos/components/tether/ble_scanner_impl.h"
 #include "chromeos/components/tether/disconnect_tethering_request_sender_impl.h"
@@ -104,12 +103,6 @@ AsynchronousShutdownObjectContainerImpl::
               ? nullptr
               : secure_channel::BleSynchronizer::Factory::Get()->BuildInstance(
                     adapter)),
-      ble_advertiser_(
-          base::FeatureList::IsEnabled(chromeos::features::kMultiDeviceApi)
-              ? nullptr
-              : BleAdvertiserImpl::Factory::NewInstance(
-                    nullptr,
-                    ble_synchronizer_.get())),
       ble_scanner_(
           base::FeatureList::IsEnabled(chromeos::features::kMultiDeviceApi)
               ? nullptr
@@ -139,7 +132,6 @@ AsynchronousShutdownObjectContainerImpl::
     ~AsynchronousShutdownObjectContainerImpl() {
   disconnect_tethering_request_sender_->RemoveObserver(this);
   if (!base::FeatureList::IsEnabled(chromeos::features::kMultiDeviceApi)) {
-    ble_advertiser_->RemoveObserver(this);
     ble_scanner_->RemoveObserver(this);
   }
 }
@@ -153,7 +145,6 @@ void AsynchronousShutdownObjectContainerImpl::Shutdown(
   // these objects. Once they notify observers that they are finished shutting
   // down, the asynchronous shutdown will complete.
   if (!base::FeatureList::IsEnabled(chromeos::features::kMultiDeviceApi)) {
-    ble_advertiser_->AddObserver(this);
     ble_scanner_->AddObserver(this);
   }
   disconnect_tethering_request_sender_->AddObserver(this);
@@ -181,11 +172,6 @@ AsynchronousShutdownObjectContainerImpl::wifi_hotspot_disconnector() {
   return wifi_hotspot_disconnector_.get();
 }
 
-void AsynchronousShutdownObjectContainerImpl::
-    OnAllAdvertisementsUnregistered() {
-  ShutdownIfPossible();
-}
-
 void AsynchronousShutdownObjectContainerImpl::
     OnPendingDisconnectRequestsComplete() {
   ShutdownIfPossible();
@@ -203,7 +189,6 @@ void AsynchronousShutdownObjectContainerImpl::ShutdownIfPossible() {
     return;
 
   if (!base::FeatureList::IsEnabled(chromeos::features::kMultiDeviceApi)) {
-    ble_advertiser_->RemoveObserver(this);
     ble_scanner_->RemoveObserver(this);
   }
   disconnect_tethering_request_sender_->RemoveObserver(this);
@@ -232,21 +217,14 @@ bool AsynchronousShutdownObjectContainerImpl::
     return true;
   }
 
-  // The BLE advertiser must unregister all of its advertisements.
-  if (!base::FeatureList::IsEnabled(chromeos::features::kMultiDeviceApi) &&
-      ble_advertiser_->AreAdvertisementsRegistered())
-    return true;
-
   return false;
 }
 
 void AsynchronousShutdownObjectContainerImpl::SetTestDoubles(
-    std::unique_ptr<BleAdvertiser> ble_advertiser,
     std::unique_ptr<BleScanner> ble_scanner,
     std::unique_ptr<DisconnectTetheringRequestSender>
         disconnect_tethering_request_sender) {
   if (!base::FeatureList::IsEnabled(chromeos::features::kMultiDeviceApi)) {
-    ble_advertiser_ = std::move(ble_advertiser);
     ble_scanner_ = std::move(ble_scanner);
   }
   disconnect_tethering_request_sender_ =

chromeos/components/tether/asynchronous_shutdown_object_container_impl.h

@@ -55,7 +55,6 @@ class WifiHotspotDisconnector;
 // Concrete AsynchronousShutdownObjectContainer implementation.
 class AsynchronousShutdownObjectContainerImpl
     : public AsynchronousShutdownObjectContainer,
-      public BleAdvertiser::Observer,
       public BleScanner::Observer,
       public DisconnectTetheringRequestSender::Observer {
  public:
@@ -114,9 +113,6 @@ class AsynchronousShutdownObjectContainerImpl
       NetworkConnectionHandler* network_connection_handler,
       PrefService* pref_service);
 
-  // BleAdvertiser::Observer:
-  void OnAllAdvertisementsUnregistered() override;
-
   // BleScanner::Observer:
   void OnDiscoverySessionStateChanged(bool discovery_session_active) override;
 
@@ -129,8 +125,7 @@ class AsynchronousShutdownObjectContainerImpl
   void ShutdownIfPossible();
   bool AreAsynchronousOperationsActive();
 
-  void SetTestDoubles(std::unique_ptr<BleAdvertiser> ble_advertiser,
-                      std::unique_ptr<BleScanner> ble_scanner,
+  void SetTestDoubles(std::unique_ptr<BleScanner> ble_scanner,
                       std::unique_ptr<DisconnectTetheringRequestSender>
                           disconnect_tethering_request_sender);
 
@@ -141,7 +136,6 @@ class AsynchronousShutdownObjectContainerImpl
       local_device_data_provider_;
   std::unique_ptr<BleAdvertisementDeviceQueue> ble_advertisement_device_queue_;
   std::unique_ptr<secure_channel::BleSynchronizerBase> ble_synchronizer_;
-  std::unique_ptr<BleAdvertiser> ble_advertiser_;
   std::unique_ptr<BleScanner> ble_scanner_;
   std::unique_ptr<BleConnectionMetricsLogger> ble_connection_metrics_logger_;
   std::unique_ptr<DisconnectTetheringRequestSender>

chromeos/components/tether/asynchronous_shutdown_object_container_impl_unittest.cc

@@ -11,7 +11,6 @@
 #include "base/test/scoped_feature_list.h"
 #include "base/test/scoped_task_environment.h"
 #include "chromeos/chromeos_features.h"
-#include "chromeos/components/tether/fake_ble_advertiser.h"
 #include "chromeos/components/tether/fake_ble_scanner.h"
 #include "chromeos/components/tether/fake_disconnect_tethering_request_sender.h"
 #include "chromeos/components/tether/fake_tether_host_fetcher.h"
@@ -100,15 +99,12 @@ class AsynchronousShutdownObjectContainerImplTest : public testing::Test {
         nullptr /* network_connection_handler */,
         test_pref_service_.get() /* pref_service */));
 
-    fake_ble_advertiser_ = new FakeBleAdvertiser(
-        false /* automatically_update_active_advertisements */);
     fake_ble_scanner_ =
         new FakeBleScanner(false /* automatically_update_discovery_session */);
     fake_disconnect_tethering_request_sender_ =
         new FakeDisconnectTetheringRequestSender();
 
     container_->SetTestDoubles(
-        base::WrapUnique(fake_ble_advertiser_),
         base::WrapUnique(fake_ble_scanner_),
         base::WrapUnique(fake_disconnect_tethering_request_sender_));
   }
@@ -137,7 +133,6 @@ class AsynchronousShutdownObjectContainerImplTest : public testing::Test {
       test_pref_service_;
   std::unique_ptr<FakeRemoteDeviceProviderFactory>
       fake_remote_device_provider_factory_;
-  FakeBleAdvertiser* fake_ble_advertiser_;
   FakeBleScanner* fake_ble_scanner_;
   FakeDisconnectTetheringRequestSender*
       fake_disconnect_tethering_request_sender_;
@@ -157,23 +152,6 @@ TEST_F(AsynchronousShutdownObjectContainerImplTest,
   EXPECT_TRUE(was_shutdown_callback_invoked_);
 }
 
-TEST_F(AsynchronousShutdownObjectContainerImplTest,
-       TestShutdown_AsyncBleAdvertiserShutdown) {
-  fake_ble_advertiser_->set_are_advertisements_registered(true);
-  EXPECT_TRUE(fake_ble_advertiser_->AreAdvertisementsRegistered());
-
-  // Start the shutdown; it should not yet succeed since there are still
-  // registered advertisements.
-  CallShutdown();
-  EXPECT_FALSE(was_shutdown_callback_invoked_);
-
-  // Now, remove these advertisements; this should cause the shutdown to
-  // complete.
-  fake_ble_advertiser_->set_are_advertisements_registered(false);
-  fake_ble_advertiser_->NotifyAllAdvertisementsUnregistered();
-  EXPECT_TRUE(was_shutdown_callback_invoked_);
-}
-
 TEST_F(AsynchronousShutdownObjectContainerImplTest,
        TestShutdown_AsyncBleScannerShutdown) {
   fake_ble_scanner_->set_is_discovery_session_active(true);
@@ -213,9 +191,6 @@ TEST_F(AsynchronousShutdownObjectContainerImplTest,
 
 TEST_F(AsynchronousShutdownObjectContainerImplTest,
        TestShutdown_MultipleSimultaneousAsyncShutdowns) {
-  fake_ble_advertiser_->set_are_advertisements_registered(true);
-  EXPECT_TRUE(fake_ble_advertiser_->AreAdvertisementsRegistered());
-
   fake_ble_scanner_->set_is_discovery_session_active(true);
   EXPECT_FALSE(fake_ble_scanner_->ShouldDiscoverySessionBeActive());
   EXPECT_TRUE(fake_ble_scanner_->IsDiscoverySessionActive());
@@ -228,13 +203,6 @@ TEST_F(AsynchronousShutdownObjectContainerImplTest,
   CallShutdown();
   EXPECT_FALSE(was_shutdown_callback_invoked_);
 
-  // Now, remove the advertisements; this should not cause the shutdown to
-  // complete since there is still an active discovery session and pending
-  // requests.
-  fake_ble_advertiser_->set_are_advertisements_registered(false);
-  fake_ble_advertiser_->NotifyAllAdvertisementsUnregistered();
-  EXPECT_FALSE(was_shutdown_callback_invoked_);
-
   // Now, remove the discovery session; this should not cause the shutdown to
   // complete since there are still pending requests.
   fake_ble_scanner_->set_is_discovery_session_active(false);
@@ -252,9 +220,6 @@ TEST_F(AsynchronousShutdownObjectContainerImplTest,
 
 TEST_F(AsynchronousShutdownObjectContainerImplTest,
        TestShutdown_MultipleSimultaneousAsyncShutdowns_BluetoothDisabled) {
-  fake_ble_advertiser_->set_are_advertisements_registered(true);
-  EXPECT_TRUE(fake_ble_advertiser_->AreAdvertisementsRegistered());
-
   fake_ble_scanner_->set_is_discovery_session_active(true);
   EXPECT_FALSE(fake_ble_scanner_->ShouldDiscoverySessionBeActive());
   EXPECT_TRUE(fake_ble_scanner_->IsDiscoverySessionActive());

chromeos/components/tether/ble_advertiser_impl.cc

-// Copyright 2017 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#include "chromeos/components/tether/ble_advertiser_impl.h"
-
-#include "base/bind.h"
-#include "base/memory/ptr_util.h"
-#include "base/threading/thread_task_runner_handle.h"
-#include "chromeos/components/proximity_auth/logging/logging.h"
-#include "chromeos/services/secure_channel/ble_service_data_helper.h"
-#include "chromeos/services/secure_channel/device_id_pair.h"
-#include "chromeos/services/secure_channel/error_tolerant_ble_advertisement_impl.h"
-#include "components/cryptauth/ble/ble_advertisement_generator.h"
-#include "components/cryptauth/proto/cryptauth_api.pb.h"
-#include "components/cryptauth/remote_device_ref.h"
-#include "device/bluetooth/bluetooth_advertisement.h"
-
-namespace chromeos {
-
-namespace tether {
-
-namespace {
-
-const char kStubLocalDeviceId[] = "N/A";
-
-// Instant Tethering does not make use of the "local device ID" argument, since
-// all connections are from the same device.
-// TODO(hansberry): Remove when SecureChannelClient migration is complete.
-secure_channel::DeviceIdPair StubDeviceIdPair(
-    const std::string& remote_device_id) {
-  return secure_channel::DeviceIdPair(remote_device_id, kStubLocalDeviceId);
-}
-
-}  // namespace
-
-// static
-BleAdvertiserImpl::Factory* BleAdvertiserImpl::Factory::factory_instance_ =
-    nullptr;
-
-// static
-std::unique_ptr<BleAdvertiser> BleAdvertiserImpl::Factory::NewInstance(
-    secure_channel::BleServiceDataHelper* ble_service_data_helper,
-    secure_channel::BleSynchronizerBase* ble_synchronizer) {
-  if (!factory_instance_)
-    factory_instance_ = new Factory();
-
-  return factory_instance_->BuildInstance(ble_service_data_helper,
-                                          ble_synchronizer);
-}
-
-// static
-void BleAdvertiserImpl::Factory::SetInstanceForTesting(Factory* factory) {
-  factory_instance_ = factory;
-}
-
-std::unique_ptr<BleAdvertiser> BleAdvertiserImpl::Factory::BuildInstance(
-    secure_channel::BleServiceDataHelper* ble_service_data_helper,
-    secure_channel::BleSynchronizerBase* ble_synchronizer) {
-  return base::WrapUnique(
-      new BleAdvertiserImpl(ble_service_data_helper, ble_synchronizer));
-}
-
-BleAdvertiserImpl::AdvertisementMetadata::AdvertisementMetadata(
-    const std::string& device_id,
-    std::unique_ptr<cryptauth::DataWithTimestamp> service_data)
-    : device_id(device_id), service_data(std::move(service_data)) {}
-
-BleAdvertiserImpl::AdvertisementMetadata::~AdvertisementMetadata() = default;
-
-BleAdvertiserImpl::BleAdvertiserImpl(
-    secure_channel::BleServiceDataHelper* ble_service_data_helper,
-    secure_channel::BleSynchronizerBase* ble_synchronizer)
-    : ble_service_data_helper_(ble_service_data_helper),
-      ble_synchronizer_(ble_synchronizer),
-      task_runner_(base::ThreadTaskRunnerHandle::Get()),
-      weak_ptr_factory_(this) {}
-
-BleAdvertiserImpl::~BleAdvertiserImpl() = default;
-
-bool BleAdvertiserImpl::StartAdvertisingToDevice(const std::string& device_id) {
-  int index_for_device = -1;
-  for (size_t i = 0; i < secure_channel::kMaxConcurrentAdvertisements; ++i) {
-    if (!registered_device_metadata_[i]) {
-      index_for_device = i;
-      break;
-    }
-  }
-
-  if (index_for_device == -1) {
-    PA_LOG(ERROR) << "Attempted to register a device when the maximum number "
-                  << "of devices have already been registered.";
-    return false;
-  }
-
-  std::unique_ptr<cryptauth::DataWithTimestamp> service_data =
-      ble_service_data_helper_->GenerateForegroundAdvertisement(
-          secure_channel::DeviceIdPair(device_id /* remote_device_id */,
-                                       kStubLocalDeviceId));
-  if (!service_data) {
-    PA_LOG(WARNING) << "Error generating advertisement for device with ID "
-                    << cryptauth::RemoteDeviceRef::TruncateDeviceIdForLogs(
-                           device_id)
-                    << ". Cannot advertise.";
-    return false;
-  }
-
-  registered_device_metadata_[index_for_device].reset(
-      new AdvertisementMetadata(device_id, std::move(service_data)));
-  UpdateAdvertisements();
-
-  return true;
-}
-
-bool BleAdvertiserImpl::StopAdvertisingToDevice(const std::string& device_id) {
-  for (auto& metadata : registered_device_metadata_) {
-    if (metadata && metadata->device_id == device_id) {
-      metadata.reset();
-      UpdateAdvertisements();
-      return true;
-    }
-  }
-
-  return false;
-}
-
-bool BleAdvertiserImpl::AreAdvertisementsRegistered() {
-  for (const auto& advertisement : advertisements_) {
-    if (advertisement)
-      return true;
-  }
-
-  return false;
-}
-
-void BleAdvertiserImpl::SetTaskRunnerForTesting(
-    scoped_refptr<base::TaskRunner> test_task_runner) {
-  task_runner_ = test_task_runner;
-}
-
-void BleAdvertiserImpl::UpdateAdvertisements() {
-  for (size_t i = 0; i < secure_channel::kMaxConcurrentAdvertisements; ++i) {
-    std::unique_ptr<AdvertisementMetadata>& metadata =
-        registered_device_metadata_[i];
-    std::unique_ptr<secure_channel::ErrorTolerantBleAdvertisement>&
-        advertisement = advertisements_[i];
-
-    // If there is a registered device but no associated advertisement, create
-    // the advertisement.
-    if (metadata && !advertisement) {
-      std::unique_ptr<cryptauth::DataWithTimestamp> service_data_copy =
-          std::make_unique<cryptauth::DataWithTimestamp>(
-              *metadata->service_data);
-      advertisements_[i] =
-          secure_channel::ErrorTolerantBleAdvertisementImpl::Factory::Get()
-              ->BuildInstance(StubDeviceIdPair(metadata->device_id),
-                              std::move(service_data_copy), ble_synchronizer_);
-      continue;
-    }
-
-    // If there is no registered device but there is an advertisement, stop it
-    // if it has not yet been stopped.
-    if (!metadata && advertisement && !advertisement->HasBeenStopped()) {
-      advertisement->Stop(base::Bind(&BleAdvertiserImpl::OnAdvertisementStopped,
-                                     weak_ptr_factory_.GetWeakPtr(),
-                                     i /* index */));
-    }
-  }
-}
-
-void BleAdvertiserImpl::OnAdvertisementStopped(size_t index) {
-  DCHECK(advertisements_[index] && advertisements_[index]->HasBeenStopped());
-  advertisements_[index].reset();
-
-  // Update advertisements, but do so as part of a new task in the run loop to
-  // prevent the possibility of a crash. See crbug.com/776241.
-  task_runner_->PostTask(
-      FROM_HERE, base::BindOnce(&BleAdvertiserImpl::UpdateAdvertisements,
-                                weak_ptr_factory_.GetWeakPtr()));
-
-  if (!AreAdvertisementsRegistered())
-    NotifyAllAdvertisementsUnregistered();
-}
-
-}  // namespace tether
-
-}  // namespace chromeos

chromeos/components/tether/ble_advertiser_impl.h

-// Copyright 2017 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#ifndef CHROMEOS_COMPONENTS_TETHER_BLE_ADVERTISER_IMPL_H_
-#define CHROMEOS_COMPONENTS_TETHER_BLE_ADVERTISER_IMPL_H_
-
-#include <array>
-
-#include "base/macros.h"
-#include "base/memory/ref_counted.h"
-#include "base/memory/weak_ptr.h"
-#include "chromeos/components/tether/ble_advertiser.h"
-#include "chromeos/services/secure_channel/ble_constants.h"
-#include "components/cryptauth/data_with_timestamp.h"
-
-namespace base {
-class TaskRunner;
-}  // namespace base
-
-namespace chromeos {
-
-namespace secure_channel {
-class BleServiceDataHelper;
-class BleSynchronizerBase;
-class ErrorTolerantBleAdvertisement;
-}  // namespace secure_channel
-
-namespace tether {
-
-// Concrete BleAdvertiser implementation.
-class BleAdvertiserImpl : public BleAdvertiser {
- public:
-  class Factory {
-   public:
-    static std::unique_ptr<BleAdvertiser> NewInstance(
-        secure_channel::BleServiceDataHelper* ble_service_data_helper,
-        secure_channel::BleSynchronizerBase* ble_synchronizer);
-    static void SetInstanceForTesting(Factory* factory);
-
-   protected:
-    virtual std::unique_ptr<BleAdvertiser> BuildInstance(
-        secure_channel::BleServiceDataHelper* ble_service_data_helper,
-        secure_channel::BleSynchronizerBase* ble_synchronizer);
-
-   private:
-    static Factory* factory_instance_;
-  };
-
-  ~BleAdvertiserImpl() override;
-
-  // BleAdvertiser:
-  bool StartAdvertisingToDevice(const std::string& device_id) override;
-  bool StopAdvertisingToDevice(const std::string& device_id) override;
-  bool AreAdvertisementsRegistered() override;
-
- protected:
-  BleAdvertiserImpl(
-      secure_channel::BleServiceDataHelper* ble_service_data_helper,
-      secure_channel::BleSynchronizerBase* ble_synchronizer);
-
- private:
-  friend class BleAdvertiserImplTest;
-
-  // Data needed to generate an advertisement.
-  struct AdvertisementMetadata {
-    AdvertisementMetadata(
-        const std::string& device_id,
-        std::unique_ptr<cryptauth::DataWithTimestamp> service_data);
-    ~AdvertisementMetadata();
-
-    std::string device_id;
-    std::unique_ptr<cryptauth::DataWithTimestamp> service_data;
-  };
-
-  void SetTaskRunnerForTesting(
-      scoped_refptr<base::TaskRunner> test_task_runner);
-  void UpdateAdvertisements();
-  void OnAdvertisementStopped(size_t index);
-
-  secure_channel::BleServiceDataHelper* ble_service_data_helper_;
-  secure_channel::BleSynchronizerBase* ble_synchronizer_;
-
-  scoped_refptr<base::TaskRunner> task_runner_;
-
-  // |registered_device_ids_| holds the device IDs that are currently
-  // registered and is always up-to-date. |advertisements_| contains the active
-  // advertisements, which may not correspond exactly to
-  // |registered_device_ids_| in the case that a previous advertisement failed
-  // to unregister.
-  std::array<std::unique_ptr<AdvertisementMetadata>,
-             secure_channel::kMaxConcurrentAdvertisements>
-      registered_device_metadata_;
-  std::array<std::unique_ptr<secure_channel::ErrorTolerantBleAdvertisement>,
-             secure_channel::kMaxConcurrentAdvertisements>
-      advertisements_;
-
-  base::WeakPtrFactory<BleAdvertiserImpl> weak_ptr_factory_;
-
-  DISALLOW_COPY_AND_ASSIGN(BleAdvertiserImpl);
-};
-
-}  // namespace tether
-
-}  // namespace chromeos
-
-#endif  // CHROMEOS_COMPONENTS_TETHER_BLE_ADVERTISER_IMPL_H_