Instant Tethering: Remove kMultiDeviceApi flagging from ConnectionPreserverImpl.

R=hansberry@chromium.org

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

chromeos/components/tether/connection_preserver_impl.cc

@@ -5,9 +5,7 @@
 #include "chromeos/components/tether/connection_preserver_impl.h"
 
 #include "base/timer/timer.h"
-#include "chromeos/chromeos_features.h"
 #include "chromeos/components/proximity_auth/logging/logging.h"
-#include "chromeos/components/tether/ble_connection_manager.h"
 #include "chromeos/components/tether/tether_host_response_recorder.h"
 #include "chromeos/network/network_state.h"
 #include "chromeos/network/network_state_handler.h"
@@ -27,17 +25,14 @@ const char kTetherFeature[] = "magic_tether";
 ConnectionPreserverImpl::ConnectionPreserverImpl(
     device_sync::DeviceSyncClient* device_sync_client,
     secure_channel::SecureChannelClient* secure_channel_client,
-    BleConnectionManager* ble_connection_manager,
     NetworkStateHandler* network_state_handler,
     ActiveHost* active_host,
     TetherHostResponseRecorder* tether_host_response_recorder)
     : device_sync_client_(device_sync_client),
       secure_channel_client_(secure_channel_client),
-      ble_connection_manager_(ble_connection_manager),
       network_state_handler_(network_state_handler),
       active_host_(active_host),
       tether_host_response_recorder_(tether_host_response_recorder),
-      request_id_(base::UnguessableToken::Create()),
       preserved_connection_timer_(std::make_unique<base::OneShotTimer>()),
       weak_ptr_factory_(this) {
   active_host_->AddObserver(this);
@@ -84,7 +79,6 @@ void ConnectionPreserverImpl::HandleSuccessfulTetherAvailabilityResponse(
 
 void ConnectionPreserverImpl::OnConnectionAttemptFailure(
     secure_channel::mojom::ConnectionAttemptFailureReason reason) {
-  DCHECK(base::FeatureList::IsEnabled(chromeos::features::kMultiDeviceApi));
   PA_LOG(WARNING) << "Failed to connect to device "
                   << cryptauth::RemoteDeviceRef::TruncateDeviceIdForLogs(
                          preserved_connection_device_id_)
@@ -94,7 +88,6 @@ void ConnectionPreserverImpl::OnConnectionAttemptFailure(
 
 void ConnectionPreserverImpl::OnConnection(
     std::unique_ptr<secure_channel::ClientChannel> channel) {
-  DCHECK(base::FeatureList::IsEnabled(chromeos::features::kMultiDeviceApi));
   PA_LOG(VERBOSE) << "Successfully preserved connection for device: "
                   << cryptauth::RemoteDeviceRef::TruncateDeviceIdForLogs(
                          preserved_connection_device_id_);
@@ -105,7 +98,6 @@ void ConnectionPreserverImpl::OnConnection(
 }
 
 void ConnectionPreserverImpl::OnDisconnected() {
-  DCHECK(base::FeatureList::IsEnabled(chromeos::features::kMultiDeviceApi));
   PA_LOG(VERBOSE) << "Remote device disconnected from this device: "
                   << cryptauth::RemoteDeviceRef::TruncateDeviceIdForLogs(
                          preserved_connection_device_id_);
@@ -171,7 +163,6 @@ void ConnectionPreserverImpl::SetPreservedConnection(
 
   preserved_connection_device_id_ = device_id;
 
-  if (base::FeatureList::IsEnabled(chromeos::features::kMultiDeviceApi)) {
   base::Optional<cryptauth::RemoteDeviceRef> remote_device =
       GetRemoteDevice(preserved_connection_device_id_);
   if (!remote_device) {
@@ -186,11 +177,6 @@ void ConnectionPreserverImpl::SetPreservedConnection(
       *remote_device, *device_sync_client_->GetLocalDeviceMetadata(),
       kTetherFeature, secure_channel::ConnectionPriority::kLow);
   connection_attempt_->SetDelegate(this);
-  } else {
-    ble_connection_manager_->RegisterRemoteDevice(
-        preserved_connection_device_id_, request_id_,
-        secure_channel::ConnectionPriority::kLow);
-  }
 
   preserved_connection_timer_->Start(
       FROM_HERE, base::TimeDelta::FromSeconds(kTimeoutSeconds),
@@ -207,13 +193,8 @@ void ConnectionPreserverImpl::RemovePreservedConnectionIfPresent() {
                          preserved_connection_device_id_)
                   << ".";
 
-  if (base::FeatureList::IsEnabled(chromeos::features::kMultiDeviceApi)) {
   connection_attempt_.reset();
   client_channel_.reset();
-  } else {
-    ble_connection_manager_->UnregisterRemoteDevice(
-        preserved_connection_device_id_, request_id_);
-  }
 
   preserved_connection_device_id_.clear();
   preserved_connection_timer_->Stop();
@@ -221,7 +202,6 @@ void ConnectionPreserverImpl::RemovePreservedConnectionIfPresent() {
 
 base::Optional<cryptauth::RemoteDeviceRef>
 ConnectionPreserverImpl::GetRemoteDevice(const std::string device_id) {
-  DCHECK(base::FeatureList::IsEnabled(chromeos::features::kMultiDeviceApi));
   for (const auto& remote_device : device_sync_client_->GetSyncedDevices()) {
     if (remote_device.GetDeviceId() == device_id)
       return remote_device;

chromeos/components/tether/connection_preserver_impl.h

@@ -24,7 +24,6 @@ class NetworkStateHandler;
 
 namespace tether {
 
-class BleConnectionManager;
 class SecureChannelClient;
 class TetherHostResponseRecorder;
 
@@ -44,7 +43,6 @@ class ConnectionPreserverImpl
   ConnectionPreserverImpl(
       device_sync::DeviceSyncClient* device_sync_client,
       secure_channel::SecureChannelClient* secure_channel_client,
-      BleConnectionManager* ble_connection_manager,
       NetworkStateHandler* network_state_handler,
       ActiveHost* active_host,
       TetherHostResponseRecorder* tether_host_response_recorder);
@@ -86,11 +84,9 @@ class ConnectionPreserverImpl
 
   device_sync::DeviceSyncClient* device_sync_client_;
   secure_channel::SecureChannelClient* secure_channel_client_;
-  BleConnectionManager* ble_connection_manager_;
   NetworkStateHandler* network_state_handler_;
   ActiveHost* active_host_;
   TetherHostResponseRecorder* tether_host_response_recorder_;
-  const base::UnguessableToken request_id_;
 
   std::unique_ptr<base::OneShotTimer> preserved_connection_timer_;
 

chromeos/components/tether/connection_preserver_impl_unittest.cc

@@ -8,12 +8,9 @@
 
 #include "base/base64.h"
 #include "base/memory/ptr_util.h"
-#include "base/test/scoped_feature_list.h"
 #include "base/test/scoped_task_environment.h"
 #include "base/timer/mock_timer.h"
-#include "chromeos/chromeos_features.h"
 #include "chromeos/components/tether/fake_active_host.h"
-#include "chromeos/components/tether/fake_ble_connection_manager.h"
 #include "chromeos/components/tether/mock_tether_host_response_recorder.h"
 #include "chromeos/components/tether/timer_factory.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
@@ -80,8 +77,6 @@ class ConnectionPreserverImplTest : public NetworkStateTest {
     fake_secure_channel_client_ =
         std::make_unique<secure_channel::FakeSecureChannelClient>();
 
-    fake_ble_connection_manager_ = std::make_unique<FakeBleConnectionManager>();
-
     fake_active_host_ = std::make_unique<FakeActiveHost>();
 
     previously_connected_host_ids_.clear();
@@ -94,8 +89,8 @@ class ConnectionPreserverImplTest : public NetworkStateTest {
 
     connection_preserver_ = std::make_unique<ConnectionPreserverImpl>(
         fake_device_sync_client_.get(), fake_secure_channel_client_.get(),
-        fake_ble_connection_manager_.get(), network_state_handler(),
-        fake_active_host_.get(), mock_tether_host_response_recorder_.get());
+        network_state_handler(), fake_active_host_.get(),
+        mock_tether_host_response_recorder_.get());
 
     mock_timer_ = new base::MockOneShotTimer();
     connection_preserver_->SetTimerForTesting(base::WrapUnique(mock_timer_));
@@ -109,42 +104,8 @@ class ConnectionPreserverImplTest : public NetworkStateTest {
     DBusThreadManager::Shutdown();
   }
 
-  void SetMultiDeviceApi(bool enabled) {
-    static const std::vector<base::Feature> kFeatures{
-        chromeos::features::kMultiDeviceApi,
-        chromeos::features::kEnableUnifiedMultiDeviceSetup};
-
-    scoped_feature_list_.InitWithFeatures(
-        (enabled ? kFeatures
-                 : std::vector<base::Feature>() /* enable_features */),
-        (enabled ? std::vector<base::Feature>()
-                 : kFeatures /* disable_features */));
-  }
-
-  void SimulateSuccessfulHostScan_MultiDeviceApiDisabled(
-      const std::string& device_id,
-      bool should_remain_registered) {
-    DCHECK(!base::FeatureList::IsEnabled(chromeos::features::kMultiDeviceApi));
-
-    base::UnguessableToken request_id = base::UnguessableToken::Create();
-    fake_ble_connection_manager_->RegisterRemoteDevice(
-        device_id, request_id, secure_channel::ConnectionPriority::kLow);
-    EXPECT_TRUE(fake_ble_connection_manager_->IsRegistered(device_id));
-
-    connection_preserver_->HandleSuccessfulTetherAvailabilityResponse(
-        device_id);
-    EXPECT_TRUE(fake_ble_connection_manager_->IsRegistered(device_id));
-
-    fake_ble_connection_manager_->UnregisterRemoteDevice(device_id, request_id);
-    EXPECT_EQ(should_remain_registered,
-              fake_ble_connection_manager_->IsRegistered(device_id));
-  }
-
-  void SimulateSuccessfulHostScan_MultiDeviceApiEnabled(
-      cryptauth::RemoteDeviceRef remote_device,
+  void SimulateSuccessfulHostScan(cryptauth::RemoteDeviceRef remote_device,
                                   bool should_remain_registered) {
-    DCHECK(base::FeatureList::IsEnabled(chromeos::features::kMultiDeviceApi));
-
     // |connection_preserver_| should only grab |fake_connection_attempt| if
     // it is intended to keep the connection open.
     auto fake_connection_attempt =
@@ -229,7 +190,6 @@ class ConnectionPreserverImplTest : public NetworkStateTest {
   std::unique_ptr<device_sync::FakeDeviceSyncClient> fake_device_sync_client_;
   std::unique_ptr<secure_channel::FakeSecureChannelClient>
       fake_secure_channel_client_;
-  std::unique_ptr<FakeBleConnectionManager> fake_ble_connection_manager_;
   std::unique_ptr<FakeActiveHost> fake_active_host_;
   std::unique_ptr<NiceMock<MockTetherHostResponseRecorder>>
       mock_tether_host_response_recorder_;
@@ -240,71 +200,33 @@ class ConnectionPreserverImplTest : public NetworkStateTest {
   std::vector<std::string> previously_connected_host_ids_;
 
  private:
-  base::test::ScopedFeatureList scoped_feature_list_;
-
   DISALLOW_COPY_AND_ASSIGN(ConnectionPreserverImplTest);
 };
 
 TEST_F(ConnectionPreserverImplTest,
        TestHandleSuccessfulTetherAvailabilityResponse_NoPreservedConnection) {
-  SetMultiDeviceApi(false /* enabled */);
-  SimulateSuccessfulHostScan_MultiDeviceApiDisabled(
-      test_remote_device_ids_[0], true /* should_remain_registered */);
-}
-
-TEST_F(
-    ConnectionPreserverImplTest,
-    TestHandleSuccessfulTetherAvailabilityResponse_NoPreservedConnection_MultiDeviceApiEnabled) {
-  SetMultiDeviceApi(true /* enabled */);
-
-  SimulateSuccessfulHostScan_MultiDeviceApiEnabled(
-      test_remote_devices_[0], true /* should_remain_registered */);
+  SimulateSuccessfulHostScan(test_remote_devices_[0],
+                             true /* should_remain_registered */);
 }
 
 TEST_F(ConnectionPreserverImplTest,
        TestHandleSuccessfulTetherAvailabilityResponse_HasInternet) {
-  SetMultiDeviceApi(false /* enabled */);
   ConnectToWifi();
 
-  SimulateSuccessfulHostScan_MultiDeviceApiDisabled(
-      test_remote_device_ids_[0], false /* should_remain_registered */);
-}
-
-TEST_F(
-    ConnectionPreserverImplTest,
-    TestHandleSuccessfulTetherAvailabilityResponse_HasInternet_MultiDeviceApiEnabled) {
-  SetMultiDeviceApi(true /* enabled */);
-
-  ConnectToWifi();
-
-  SimulateSuccessfulHostScan_MultiDeviceApiEnabled(
-      test_remote_devices_[0], false /* should_remain_registered */);
+  SimulateSuccessfulHostScan(test_remote_devices_[0],
+                             false /* should_remain_registered */);
 }
 
 TEST_F(
     ConnectionPreserverImplTest,
     TestHandleSuccessfulTetherAvailabilityResponse_PreservedConnectionExists_NoPreviouslyConnectedHosts) {
-  SetMultiDeviceApi(false /* enabled */);
-  SimulateSuccessfulHostScan_MultiDeviceApiDisabled(
-      test_remote_device_ids_[0], true /* should_remain_registered */);
-  SimulateSuccessfulHostScan_MultiDeviceApiDisabled(
-      test_remote_device_ids_[1], true /* should_remain_registered */);
-  EXPECT_FALSE(
-      fake_ble_connection_manager_->IsRegistered(test_remote_device_ids_[0]));
-}
-
-TEST_F(
-    ConnectionPreserverImplTest,
-    TestHandleSuccessfulTetherAvailabilityResponse_PreservedConnectionExists_NoPreviouslyConnectedHosts_MultiDeviceApiEnabled) {
-  SetMultiDeviceApi(true /* enabled */);
-
-  SimulateSuccessfulHostScan_MultiDeviceApiEnabled(
-      test_remote_devices_[0], true /* should_remain_registered */);
+  SimulateSuccessfulHostScan(test_remote_devices_[0],
+                             true /* should_remain_registered */);
   VerifyChannelForRemoteDeviceDestroyed(test_remote_devices_[0],
                                         false /* expect_destroyed */);
 
-  SimulateSuccessfulHostScan_MultiDeviceApiEnabled(
-      test_remote_devices_[1], true /* should_remain_registered */);
+  SimulateSuccessfulHostScan(test_remote_devices_[1],
+                             true /* should_remain_registered */);
   VerifyChannelForRemoteDeviceDestroyed(test_remote_devices_[0],
                                         true /* expect_destroyed */);
   VerifyChannelForRemoteDeviceDestroyed(test_remote_devices_[1],
@@ -313,22 +235,8 @@ TEST_F(
 
 TEST_F(ConnectionPreserverImplTest,
        TestHandleSuccessfulTetherAvailabilityResponse_TimesOut) {
-  SetMultiDeviceApi(false /* enabled */);
-  SimulateSuccessfulHostScan_MultiDeviceApiDisabled(
-      test_remote_device_ids_[0], true /* should_remain_registered */);
-
-  mock_timer_->Fire();
-  EXPECT_FALSE(
-      fake_ble_connection_manager_->IsRegistered(test_remote_device_ids_[0]));
-}
-
-TEST_F(
-    ConnectionPreserverImplTest,
-    TestHandleSuccessfulTetherAvailabilityResponse_TimesOut_MultiDeviceApiEnabled) {
-  SetMultiDeviceApi(true /* enabled */);
-
-  SimulateSuccessfulHostScan_MultiDeviceApiEnabled(
-      test_remote_devices_[0], true /* should_remain_registered */);
+  SimulateSuccessfulHostScan(test_remote_devices_[0],
+                             true /* should_remain_registered */);
 
   mock_timer_->Fire();
   VerifyChannelForRemoteDeviceDestroyed(test_remote_devices_[0],
@@ -337,22 +245,8 @@ TEST_F(
 
 TEST_F(ConnectionPreserverImplTest,
        TestHandleSuccessfulTetherAvailabilityResponse_PreserverDestroyed) {
-  SetMultiDeviceApi(false /* enabled */);
-  SimulateSuccessfulHostScan_MultiDeviceApiDisabled(
-      test_remote_device_ids_[0], true /* should_remain_registered */);
-
-  connection_preserver_.reset();
-  EXPECT_FALSE(
-      fake_ble_connection_manager_->IsRegistered(test_remote_device_ids_[0]));
-}
-
-TEST_F(
-    ConnectionPreserverImplTest,
-    TestHandleSuccessfulTetherAvailabilityResponse_PreserverDestroyed_MultiDeviceApiEnabled) {
-  SetMultiDeviceApi(true /* enabled */);
-
-  SimulateSuccessfulHostScan_MultiDeviceApiEnabled(
-      test_remote_devices_[0], true /* should_remain_registered */);
+  SimulateSuccessfulHostScan(test_remote_devices_[0],
+                             true /* should_remain_registered */);
 
   connection_preserver_.reset();
   VerifyChannelForRemoteDeviceDestroyed(test_remote_devices_[0],
@@ -362,25 +256,8 @@ TEST_F(
 TEST_F(
     ConnectionPreserverImplTest,
     TestHandleSuccessfulTetherAvailabilityResponse_ActiveHostBecomesConnected) {
-  SetMultiDeviceApi(false /* enabled */);
-  SimulateSuccessfulHostScan_MultiDeviceApiDisabled(
-      test_remote_device_ids_[0], true /* should_remain_registered */);
-
-  fake_active_host_->SetActiveHostConnecting(test_remote_device_ids_[0],
-                                             kTetherNetworkGuid);
-  fake_active_host_->SetActiveHostConnected(
-      test_remote_device_ids_[0], kTetherNetworkGuid, kWifiNetworkGuid);
-  EXPECT_FALSE(
-      fake_ble_connection_manager_->IsRegistered(test_remote_device_ids_[0]));
-}
-
-TEST_F(
-    ConnectionPreserverImplTest,
-    TestHandleSuccessfulTetherAvailabilityResponse_ActiveHostBecomesConnected_MultiDeviceApiEnabled) {
-  SetMultiDeviceApi(true /* enabled */);
-
-  SimulateSuccessfulHostScan_MultiDeviceApiEnabled(
-      test_remote_devices_[0], true /* should_remain_registered */);
+  SimulateSuccessfulHostScan(test_remote_devices_[0],
+                             true /* should_remain_registered */);
 
   fake_active_host_->SetActiveHostConnecting(test_remote_device_ids_[0],
                                              kTetherNetworkGuid);
@@ -394,73 +271,37 @@ TEST_F(
 TEST_F(
     ConnectionPreserverImplTest,
     TestHandleSuccessfulTetherAvailabilityResponse_PreviouslyConnectedHostsExist) {
-  SetMultiDeviceApi(false /* enabled */);
-
-  // |test_remote_device_ids_[0]| is the most recently connected device, and
-  // should be preferred over any other device.
-  previously_connected_host_ids_.push_back(test_remote_device_ids_[0]);
-  previously_connected_host_ids_.push_back(test_remote_device_ids_[1]);
-
-  SimulateSuccessfulHostScan_MultiDeviceApiDisabled(
-      test_remote_device_ids_[2], true /* should_remain_registered */);
-
-  SimulateSuccessfulHostScan_MultiDeviceApiDisabled(
-      test_remote_device_ids_[1], true /* should_remain_registered */);
-  EXPECT_FALSE(
-      fake_ble_connection_manager_->IsRegistered(test_remote_device_ids_[2]));
-
-  SimulateSuccessfulHostScan_MultiDeviceApiDisabled(
-      test_remote_device_ids_[0], true /* should_remain_registered */);
-  EXPECT_FALSE(
-      fake_ble_connection_manager_->IsRegistered(test_remote_device_ids_[1]));
-
-  SimulateSuccessfulHostScan_MultiDeviceApiDisabled(
-      test_remote_device_ids_[1], false /* should_remain_registered */);
-  EXPECT_TRUE(
-      fake_ble_connection_manager_->IsRegistered(test_remote_device_ids_[0]));
-
-  SimulateSuccessfulHostScan_MultiDeviceApiDisabled(
-      test_remote_device_ids_[2], false /* should_remain_registered */);
-  EXPECT_TRUE(
-      fake_ble_connection_manager_->IsRegistered(test_remote_device_ids_[0]));
-}
-
-TEST_F(
-    ConnectionPreserverImplTest,
-    TestHandleSuccessfulTetherAvailabilityResponse_PreviouslyConnectedHostsExist_MultiDeviceApiEnabled) {
-  SetMultiDeviceApi(true /* enabled */);
-
   // |test_remote_device_ids_[0]| is the most recently connected device, and
   // should be preferred over any other device.
   previously_connected_host_ids_.push_back(test_remote_device_ids_[0]);
   previously_connected_host_ids_.push_back(test_remote_device_ids_[1]);
 
-  SimulateSuccessfulHostScan_MultiDeviceApiEnabled(
-      test_remote_devices_[2], true /* should_remain_registered */);
+  SimulateSuccessfulHostScan(test_remote_devices_[2],
+                             true /* should_remain_registered */);
   VerifyChannelForRemoteDeviceDestroyed(test_remote_devices_[2],
                                         false /* expect_destroyed */);
 
-  SimulateSuccessfulHostScan_MultiDeviceApiEnabled(
-      test_remote_devices_[1], true /* should_remain_registered */);
+  SimulateSuccessfulHostScan(test_remote_devices_[1],
+                             true /* should_remain_registered */);
   VerifyChannelForRemoteDeviceDestroyed(test_remote_devices_[2],
                                         true /* expect_destroyed */);
   VerifyChannelForRemoteDeviceDestroyed(test_remote_devices_[1],
                                         false /* expect_destroyed */);
 
-  SimulateSuccessfulHostScan_MultiDeviceApiEnabled(
-      test_remote_devices_[0], true /* should_remain_registered */);
+  SimulateSuccessfulHostScan(test_remote_devices_[0],
+                             true /* should_remain_registered */);
   VerifyChannelForRemoteDeviceDestroyed(test_remote_devices_[1],
                                         true /* expect_destroyed */);
   VerifyChannelForRemoteDeviceDestroyed(test_remote_devices_[0],
                                         false /* expect_destroyed */);
 
-  SimulateSuccessfulHostScan_MultiDeviceApiEnabled(
-      test_remote_devices_[1], false /* should_remain_registered */);
+  SimulateSuccessfulHostScan(test_remote_devices_[1],
+                             false /* should_remain_registered */);
   VerifyChannelForRemoteDeviceDestroyed(test_remote_devices_[0],
                                         false /* expect_destroyed */);
 
-  SimulateSuccessfulHostScan_MultiDeviceApiEnabled(
-      test_remote_devices_[2], false /* should_remain_registered */);
+  SimulateSuccessfulHostScan(test_remote_devices_[2],
+                             false /* should_remain_registered */);
   VerifyChannelForRemoteDeviceDestroyed(test_remote_devices_[0],
                                         false /* expect_destroyed */);
 }

chromeos/components/tether/synchronous_shutdown_object_container_impl.cc

@@ -154,7 +154,6 @@ SynchronousShutdownObjectContainerImpl::SynchronousShutdownObjectContainerImpl(
       connection_preserver_(std::make_unique<ConnectionPreserverImpl>(
           device_sync_client,
           secure_channel_client,
-          asychronous_container->ble_connection_manager(),
           network_state_handler_,
           active_host_.get(),
           tether_host_response_recorder_.get())),