Phonehub: Hide UI in lockscreen

- Phonehub will now be hidden in lockscreen, preventing notifications
  and controls from appearing in the lockscreen.
- A followup CL will handle the suspend state of the Chrome OS device.

Bug: 1139415, 1106937
Test: chromeos_components_unittest, ash_unittests
Change-Id: Ie06dc84e22db98032e4f1479f4db5e4df0c1290d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2516248Reviewed-by: Achuith Bhandarkar <achuith@chromium.org>
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Commit-Queue: Jimmy Gong <jimmyxgong@chromium.org>
Cr-Commit-Position: refs/heads/master@{#825030}

ash/system/phonehub/phone_hub_ui_controller.cc

@@ -147,6 +147,8 @@ PhoneHubUiController::GetUiStateFromPhoneHubManager() {
       return UiState::kPhoneConnecting;
     case FeatureStatus::kEnabledAndConnected:
       return UiState::kPhoneConnected;
+    case FeatureStatus::kUnavailableScreenLocked:
+      return UiState::kHidden;
   }
 }
 

ash/system/phonehub/phone_hub_ui_controller_unittest.cc

@@ -144,4 +144,11 @@ TEST_F(PhoneHubUiControllerTest, PhoneConnected) {
   EXPECT_EQ(kPhoneConnectedView, content_view->GetID());
 }
 
+TEST_F(PhoneHubUiControllerTest, UnavailableScreenLocked) {
+  GetFeatureStatusProvider()->SetStatus(
+      FeatureStatus::kUnavailableScreenLocked);
+  EXPECT_EQ(PhoneHubUiController::UiState::kHidden, controller_.ui_state());
+  EXPECT_FALSE(controller_.CreateContentView(/*bubble_view=*/nullptr).get());
+}
+
 }  // namespace ash

chromeos/components/phonehub/BUILD.gn

@@ -95,6 +95,7 @@ static_library("phonehub") {
     "//chromeos/services/secure_channel/public/mojom",
     "//components/keyed_service/core",
     "//components/prefs",
+    "//components/session_manager/core",
     "//device/bluetooth",
     "//net",
     "//ui/gfx",
@@ -201,6 +202,7 @@ source_set("unit_tests") {
     "//chromeos/services/network_config/public/cpp:test_support",
     "//chromeos/services/secure_channel/public/cpp/client:test_support",
     "//components/prefs:test_support",
+    "//components/session_manager/core",
     "//device/bluetooth:mocks",
     "//testing/gtest",
     "//ui/gfx",

chromeos/components/phonehub/DEPS

@@ -7,6 +7,7 @@ include_rules = [
   "+chromeos/services/secure_channel/public/cpp/client",
   "+components/keyed_service/core/keyed_service.h",
   "+components/prefs",
+  "+components/session_manager/core",
   "+device/bluetooth",
   "+net",
   "+ui/gfx",

chromeos/components/phonehub/connection_manager.h

@@ -49,6 +49,10 @@ class ConnectionManager {
   // the connection.
   virtual void AttemptConnection() = 0;
 
+  // Cancels an ongoing connection attempt. Is a no-op is there is currently no
+  // connection attempt.
+  virtual void Disconnect() = 0;
+
   // Sends a message with the specified |payload|.
   virtual void SendMessage(const std::string& payload) = 0;
 

chromeos/components/phonehub/connection_manager_impl.cc

@@ -149,7 +149,7 @@ void ConnectionManagerImpl::AttemptConnection() {
   }
   connection_attempt_->SetDelegate(this);
 
-  PA_LOG(INFO) << "ConnectiongManager status updated to: " << GetStatus();
+  PA_LOG(INFO) << "ConnectionManager status updated to: " << GetStatus();
   NotifyStatusChanged();
 
   timer_->Start(FROM_HERE, kConnectionTimeoutSeconds,
@@ -157,6 +157,11 @@ void ConnectionManagerImpl::AttemptConnection() {
                                weak_ptr_factory_.GetWeakPtr()));
 }
 
+void ConnectionManagerImpl::Disconnect() {
+  PA_LOG(INFO) << "ConnectionManager disconnecting connection.";
+  TearDownConnection();
+}
+
 void ConnectionManagerImpl::SendMessage(const std::string& payload) {
   if (!channel_) {
     PA_LOG(ERROR) << "SendMessage() failed because channel is null.";
@@ -186,12 +191,7 @@ void ConnectionManagerImpl::OnConnection(
 }
 
 void ConnectionManagerImpl::OnDisconnected() {
-  // Stop timer in case we are disconnected before the connection timed out.
-  timer_->Stop();
-  connection_attempt_.reset();
-  channel_->RemoveObserver(this);
-  channel_.reset();
-  NotifyStatusChanged();
+  TearDownConnection();
 }
 
 void ConnectionManagerImpl::OnMessageReceived(const std::string& payload) {
@@ -206,5 +206,15 @@ void ConnectionManagerImpl::OnConnectionTimeout() {
   NotifyStatusChanged();
 }
 
+void ConnectionManagerImpl::TearDownConnection() {
+  // Stop timer in case we are disconnected before the connection timed out.
+  timer_->Stop();
+  connection_attempt_.reset();
+  if (channel_)
+    channel_->RemoveObserver(this);
+  channel_.reset();
+  NotifyStatusChanged();
+}
+
 }  // namespace phonehub
 }  // namespace chromeos

chromeos/components/phonehub/connection_manager_impl.h

@@ -48,6 +48,7 @@ class ConnectionManagerImpl
   // ConnectionManager:
   ConnectionManager::Status GetStatus() const override;
   void AttemptConnection() override;
+  void Disconnect() override;
   void SendMessage(const std::string& payload) override;
 
  private:
@@ -91,6 +92,8 @@ class ConnectionManagerImpl
 
   void OnConnectionTimeout();
 
+  void TearDownConnection();
+
   multidevice_setup::MultiDeviceSetupClient* multidevice_setup_client_;
 
   device_sync::DeviceSyncClient* device_sync_client_;

chromeos/components/phonehub/connection_manager_impl_unittest.cc

@@ -346,5 +346,24 @@ TEST_F(ConnectionManagerImplTest, ConnectionTimeout) {
                                       1);
 }
 
+TEST_F(ConnectionManagerImplTest, DisconnectConnection) {
+  CreateFakeConnectionAttempt();
+  connection_manager_->AttemptConnection();
+
+  // Status has been updated to connecting, verify that the status observer
+  // has been called.
+  EXPECT_EQ(1u, GetNumStatusObserverCalls());
+  EXPECT_EQ(ConnectionManager::Status::kConnecting, GetStatus());
+  VerifyTimerSet();
+
+  // Disconnect the connection attempt.
+  connection_manager_->Disconnect();
+  EXPECT_EQ(2u, GetNumStatusObserverCalls());
+  EXPECT_EQ(ConnectionManager::Status::kDisconnected, GetStatus());
+  VerifyTimerStopped();
+  histogram_tester_.ExpectBucketCount("PhoneHub.Connectivity.Success", false,
+                                      1);
+}
+
 }  // namespace phonehub
 }  // namespace chromeos

chromeos/components/phonehub/connection_scheduler_impl.cc

@@ -58,12 +58,10 @@ void ConnectionSchedulerImpl::OnFeatureStatusChanged() {
   current_feature_status_ = feature_status_provider_->GetStatus();
 
   switch (current_feature_status_) {
-    // The following states indicate either the feature state of the devices
-    // changed or if a connection is established between the devices. In the
-    // case where the feature state has been changed, we do not want to
-    // schedule a new connection attempt until the devices are available to
-    // connect. If a connection is established, we also do not want to schedule
-    // a new connection. Reset the backoff and return early.
+    // The following feature states indicate that there is an interruption with
+    // establishing connection to the host phone or that the feature is blocked
+    // from initiating a connection. Disconnect the existing connection, reset
+    // backoffs, and return early.
     case FeatureStatus::kNotEligibleForFeature:
       FALLTHROUGH;
     case FeatureStatus::kEligiblePhoneButNotSetUp:
@@ -74,11 +72,17 @@ void ConnectionSchedulerImpl::OnFeatureStatusChanged() {
       FALLTHROUGH;
     case FeatureStatus::kUnavailableBluetoothOff:
       FALLTHROUGH;
+    case FeatureStatus::kUnavailableScreenLocked:
+      DisconnectAndClearBackoffAttempts();
+      return;
+
+    // Connection has been established, clear existing backoffs and return
+    // early.
     case FeatureStatus::kEnabledAndConnected:
       ClearBackoffAttempts();
       return;
 
-    // Connection in progress, waiting for the next status update.
+    // Connection is in progress, return and wait for the result.
     case FeatureStatus::kEnabledAndConnecting:
       return;
 
@@ -115,6 +119,13 @@ void ConnectionSchedulerImpl::ClearBackoffAttempts() {
   retry_backoff_.Reset();
 }
 
+void ConnectionSchedulerImpl::DisconnectAndClearBackoffAttempts() {
+  ClearBackoffAttempts();
+
+  // Disconnect existing connection or connection attempt.
+  connection_manager_->Disconnect();
+}
+
 base::TimeDelta
 ConnectionSchedulerImpl::GetCurrentBackoffDelayTimeForTesting() {
   return retry_backoff_.GetTimeUntilRelease();

chromeos/components/phonehub/connection_scheduler_impl.h

@@ -34,7 +34,10 @@ class ConnectionSchedulerImpl : public ConnectionScheduler,
   // FeatureStatusProvider::Observer:
   void OnFeatureStatusChanged() override;
 
-  // Invalidate all pending backoff attempts.
+  // Invalidate all pending backoff attempts and disconnects the current
+  // connection attempt.
+  void DisconnectAndClearBackoffAttempts();
+
   void ClearBackoffAttempts();
 
   // Test only functions.

chromeos/components/phonehub/connection_scheduler_impl_unittest.cc

@@ -143,6 +143,8 @@ TEST_F(ConnectionSchedulerImplTest, BackoffRetryWithUpdatedFeatures) {
   // connection.
   EXPECT_EQ(0, GetBackoffFailureCount());
   EXPECT_EQ(1u, fake_connection_manager_->num_attempt_connection_calls());
+  // Expect that connection has been disconnected.
+  EXPECT_EQ(1u, fake_connection_manager_->num_disconnect_calls());
 
   // Fast forward time and confirm no other retries have been made.
   task_environment_.FastForwardBy(base::TimeDelta::FromSeconds(100));
@@ -167,5 +169,26 @@ TEST_F(ConnectionSchedulerImplTest, BackoffRetryWithUpdatedFeatures) {
   // failure count.
   EXPECT_EQ(1, GetBackoffFailureCount());
 }
+
+TEST_F(ConnectionSchedulerImplTest, ScheduleConnectionAfterUnlock) {
+  fake_feature_status_provider_->SetStatus(
+      FeatureStatus::kEnabledButDisconnected);
+  CreateConnectionScheduler();
+
+  // Simulate screen locked and expect no scheduled connections.
+  fake_feature_status_provider_->SetStatus(
+      FeatureStatus::kUnavailableScreenLocked);
+  // Expect no scheduled connections on screen lock.
+  EXPECT_EQ(0, GetBackoffFailureCount());
+  EXPECT_EQ(0u, fake_connection_manager_->num_attempt_connection_calls());
+  EXPECT_EQ(1u, fake_connection_manager_->num_disconnect_calls());
+
+  // Simulate screen unlocked and expect a scheduled connection.
+  fake_feature_status_provider_->SetStatus(
+      FeatureStatus::kEnabledButDisconnected);
+  EXPECT_EQ(0, GetBackoffFailureCount());
+  EXPECT_EQ(1u, fake_connection_manager_->num_attempt_connection_calls());
+}
+
 }  // namespace phonehub
 }  // namespace chromeos
\ No newline at end of file

chromeos/components/phonehub/fake_connection_manager.cc

@@ -30,6 +30,11 @@ void FakeConnectionManager::AttemptConnection() {
     SetStatus(Status::kConnecting);
 }
 
+void FakeConnectionManager::Disconnect() {
+  ++num_disconnect_calls_;
+  SetStatus(Status::kDisconnected);
+}
+
 void FakeConnectionManager::SendMessage(const std::string& payload) {
   sent_messages_.push_back(payload);
 }

chromeos/components/phonehub/fake_connection_manager.h

@@ -26,15 +26,19 @@ class FakeConnectionManager : public ConnectionManager {
     return num_attempt_connection_calls_;
   }
 
+  size_t num_disconnect_calls() const { return num_disconnect_calls_; }
+
  private:
   // ConnectionManager:
   Status GetStatus() const override;
   void AttemptConnection() override;
+  void Disconnect() override;
   void SendMessage(const std::string& payload) override;
 
   Status status_;
   std::vector<std::string> sent_messages_;
   size_t num_attempt_connection_calls_ = 0;
+  size_t num_disconnect_calls_ = 0;
 };
 
 }  // namespace phonehub

chromeos/components/phonehub/feature_status.cc

@@ -33,6 +33,9 @@ std::ostream& operator<<(std::ostream& stream, FeatureStatus status) {
     case FeatureStatus::kEnabledAndConnected:
       stream << "[Enabled; connected]";
       break;
+    case FeatureStatus::kUnavailableScreenLocked:
+      stream << "[Unavailable; screen locked]";
+      break;
   }
 
   return stream;

chromeos/components/phonehub/feature_status.h

@@ -51,8 +51,11 @@ enum class FeatureStatus {
   // The feature is enabled, and there is an active connection with the phone.
   kEnabledAndConnected = 7,
 
+  // The feature is unavailable during the lockscreen.
+  kUnavailableScreenLocked = 8,
+
   // Max value needed for metrics.
-  kMaxValue = kEnabledAndConnected
+  kMaxValue = kUnavailableScreenLocked,
 };
 
 std::ostream& operator<<(std::ostream& stream, FeatureStatus status);

chromeos/components/phonehub/feature_status_provider_impl.cc

@@ -117,13 +117,17 @@ bool IsFeatureDisabledByUser(FeatureState feature_state) {
 FeatureStatusProviderImpl::FeatureStatusProviderImpl(
     device_sync::DeviceSyncClient* device_sync_client,
     multidevice_setup::MultiDeviceSetupClient* multidevice_setup_client,
-    ConnectionManager* connection_manager)
+    ConnectionManager* connection_manager,
+    session_manager::SessionManager* session_manager)
     : device_sync_client_(device_sync_client),
       multidevice_setup_client_(multidevice_setup_client),
-      connection_manager_(connection_manager) {
+      connection_manager_(connection_manager),
+      session_manager_(session_manager) {
+  DCHECK(session_manager_);
   device_sync_client_->AddObserver(this);
   multidevice_setup_client_->AddObserver(this);
   connection_manager_->AddObserver(this);
+  session_manager_->AddObserver(this);
 
   device::BluetoothAdapterFactory::Get()->GetAdapter(
       base::BindOnce(&FeatureStatusProviderImpl::OnBluetoothAdapterReceived,
@@ -138,6 +142,7 @@ FeatureStatusProviderImpl::~FeatureStatusProviderImpl() {
   connection_manager_->RemoveObserver(this);
   if (bluetooth_adapter_)
     bluetooth_adapter_->RemoveObserver(this);
+  session_manager_->RemoveObserver(this);
 }
 
 FeatureStatus FeatureStatusProviderImpl::GetStatus() const {
@@ -204,6 +209,10 @@ void FeatureStatusProviderImpl::OnConnectionStatusChanged() {
   UpdateStatus();
 }
 
+void FeatureStatusProviderImpl::OnSessionStateChanged() {
+  UpdateStatus();
+}
+
 void FeatureStatusProviderImpl::UpdateStatus() {
   DCHECK(status_.has_value());
 
@@ -237,6 +246,9 @@ FeatureStatus FeatureStatusProviderImpl::ComputeStatus() {
     return FeatureStatus::kNotEligibleForFeature;
   }
 
+  if (session_manager_->IsScreenLocked())
+    return FeatureStatus::kUnavailableScreenLocked;
+
   HostStatus host_status = multidevice_setup_client_->GetHostStatus().first;
 
   if (host_status == HostStatus::kEligibleHostExistsButNoHostSet)

chromeos/components/phonehub/feature_status_provider_impl.h

@@ -11,6 +11,8 @@
 #include "chromeos/components/phonehub/feature_status_provider.h"
 #include "chromeos/services/device_sync/public/cpp/device_sync_client.h"
 #include "chromeos/services/multidevice_setup/public/cpp/multidevice_setup_client.h"
+#include "components/session_manager/core/session_manager.h"
+#include "components/session_manager/core/session_manager_observer.h"
 #include "device/bluetooth/bluetooth_adapter.h"
 
 namespace chromeos {
@@ -24,12 +26,14 @@ class FeatureStatusProviderImpl
       public device_sync::DeviceSyncClient::Observer,
       public multidevice_setup::MultiDeviceSetupClient::Observer,
       public device::BluetoothAdapter::Observer,
-      public ConnectionManager::Observer {
+      public ConnectionManager::Observer,
+      public session_manager::SessionManagerObserver {
  public:
   FeatureStatusProviderImpl(
       device_sync::DeviceSyncClient* device_sync_client,
       multidevice_setup::MultiDeviceSetupClient* multidevice_setup_client,
-      ConnectionManager* connection_manager);
+      ConnectionManager* connection_manager,
+      session_manager::SessionManager* session_manager);
   ~FeatureStatusProviderImpl() override;
 
  private:
@@ -65,11 +69,15 @@ class FeatureStatusProviderImpl
   // ConnectionManager::Observer:
   void OnConnectionStatusChanged() override;
 
+  // SessionManagerObserver:
+  void OnSessionStateChanged() override;
+
   void RecordFeatureStatusOnLogin();
 
   device_sync::DeviceSyncClient* device_sync_client_;
   multidevice_setup::MultiDeviceSetupClient* multidevice_setup_client_;
   ConnectionManager* connection_manager_;
+  session_manager::SessionManager* session_manager_;
 
   scoped_refptr<device::BluetoothAdapter> bluetooth_adapter_;
   base::Optional<FeatureStatus> status_;

chromeos/components/phonehub/feature_status_provider_impl_unittest.cc

@@ -11,6 +11,7 @@
 #include "chromeos/components/phonehub/fake_connection_manager.h"
 #include "chromeos/services/device_sync/public/cpp/fake_device_sync_client.h"
 #include "chromeos/services/multidevice_setup/public/cpp/fake_multidevice_setup_client.h"
+#include "components/session_manager/core/session_manager.h"
 #include "device/bluetooth/bluetooth_adapter_factory.h"
 #include "device/bluetooth/test/mock_bluetooth_adapter.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -102,9 +103,10 @@ class FeatureStatusProviderImplTest : public testing::Test {
     device::BluetoothAdapterFactory::SetAdapterForTesting(mock_adapter_);
     fake_device_sync_client_.NotifyReady();
 
+    session_manager_ = std::make_unique<session_manager::SessionManager>();
     provider_ = std::make_unique<FeatureStatusProviderImpl>(
         &fake_device_sync_client_, &fake_multidevice_setup_client_,
-        &fake_connection_manager_);
+        &fake_connection_manager_, session_manager_.get());
     provider_->AddObserver(&fake_observer_);
   }
 
@@ -170,6 +172,10 @@ class FeatureStatusProviderImplTest : public testing::Test {
 
   size_t GetNumObserverCalls() const { return fake_observer_.num_calls(); }
 
+  session_manager::SessionManager* session_manager() {
+    return session_manager_.get();
+  }
+
  private:
   bool is_adapter_present() { return is_adapter_present_; }
   bool is_adapter_powered() { return is_adapter_powered_; }
@@ -186,6 +192,7 @@ class FeatureStatusProviderImplTest : public testing::Test {
   bool is_adapter_powered_ = true;
 
   FakeObserver fake_observer_;
+  std::unique_ptr<session_manager::SessionManager> session_manager_;
   std::unique_ptr<FeatureStatusProvider> provider_;
 };
 
@@ -370,6 +377,16 @@ TEST_F(FeatureStatusProviderImplTest, TransitionBetweenAllStatuses) {
   SetConnectionStatus(ConnectionManager::Status::kDisconnected);
   EXPECT_EQ(FeatureStatus::kEnabledButDisconnected, GetStatus());
   EXPECT_EQ(8u, GetNumObserverCalls());
+
+  // Simulate lock screen displayed.
+  session_manager()->SetSessionState(session_manager::SessionState::LOCKED);
+  EXPECT_EQ(FeatureStatus::kUnavailableScreenLocked, GetStatus());
+  EXPECT_EQ(9u, GetNumObserverCalls());
+
+  // Simulate user unlocks the device.
+  session_manager()->SetSessionState(session_manager::SessionState::ACTIVE);
+  EXPECT_EQ(FeatureStatus::kEnabledButDisconnected, GetStatus());
+  EXPECT_EQ(10u, GetNumObserverCalls());
 }
 
 TEST_F(FeatureStatusProviderImplTest, AttemptingConnection) {
@@ -413,5 +430,22 @@ TEST_F(FeatureStatusProviderImplTest, AttemptedConnectionFailed) {
   EXPECT_EQ(3u, GetNumObserverCalls());
 }
 
+TEST_F(FeatureStatusProviderImplTest, LockScreenStatusUpdate) {
+  SetEligibleSyncedDevices();
+  SetMultiDeviceState(HostStatus::kHostVerified, FeatureState::kEnabledByUser);
+  EXPECT_EQ(FeatureStatus::kEnabledButDisconnected, GetStatus());
+  EXPECT_EQ(1u, GetNumObserverCalls());
+
+  // Simulate lock screen displayed.
+  session_manager()->SetSessionState(session_manager::SessionState::LOCKED);
+  EXPECT_EQ(FeatureStatus::kUnavailableScreenLocked, GetStatus());
+  EXPECT_EQ(2u, GetNumObserverCalls());
+
+  // Simulate user unlocks the device.
+  session_manager()->SetSessionState(session_manager::SessionState::ACTIVE);
+  EXPECT_EQ(FeatureStatus::kEnabledButDisconnected, GetStatus());
+  EXPECT_EQ(3u, GetNumObserverCalls());
+}
+
 }  // namespace phonehub
 }  // namespace chromeos

chromeos/components/phonehub/phone_hub_manager_impl.cc

@@ -23,6 +23,7 @@
 #include "chromeos/components/phonehub/phone_model.h"
 #include "chromeos/components/phonehub/phone_status_processor.h"
 #include "chromeos/components/phonehub/tether_controller_impl.h"
+#include "components/session_manager/core/session_manager.h"
 
 namespace chromeos {
 namespace phonehub {
@@ -41,7 +42,8 @@ PhoneHubManagerImpl::PhoneHubManagerImpl(
       feature_status_provider_(std::make_unique<FeatureStatusProviderImpl>(
           device_sync_client,
           multidevice_setup_client,
-          connection_manager_.get())),
+          connection_manager_.get(),
+          session_manager::SessionManager::Get())),
       message_receiver_(
           std::make_unique<MessageReceiverImpl>(connection_manager_.get())),
       message_sender_(

tools/metrics/histograms/enums.xml

@@ -57839,6 +57839,7 @@ Called by update_net_trust_anchors.py.-->
   <int value="5" label="Unavalable Bluetooth disconnected"/>
   <int value="6" label="Enabled and connecting"/>
   <int value="7" label="Enabled and connected"/>
+  <int value="8" label="Unavailable screen locked"/>
 </enum>
 
 <enum name="PhoneHubInterstitialScreenEvent">