[HPS] disable HpsSense in tablet mode.

(1) let HpsSenseController observe HpsOrientationController.

(2) refactor HpsSenseController to only have one configuration
    function ReconfigViaDbus.
(3) unit tests are fixed accordingly.

BUG=b:222585427
TEST='All unit tests passed. Also manually tested on brya.'

Change-Id: Ic2856510d80bd7b2e8d9ddfd47edeb35da37618e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3552146
Reviewed-by: Michael Martis <martis@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Commit-Queue: Guoxing Zhao <charleszhao@chromium.org>
Cr-Commit-Position: refs/heads/main@{#988743}

ash/shell.cc

@@ -713,6 +713,9 @@ Shell::~Shell() {
   // before destroying |session_controller_|.
   accelerator_controller_->Shutdown();
 
+  // Must be destructed before hps_orientation_controller_.
+  power_prefs_.reset();
+
   // Must be destructed before the tablet mode and message center controllers,
   // both of which these rely on.
   hps_notify_controller_.reset();
@@ -787,7 +790,6 @@ Shell::~Shell() {
   event_client_.reset();
   toplevel_window_event_handler_.reset();
   visibility_controller_.reset();
-  power_prefs_.reset();
 
   tray_action_.reset();
 
@@ -1009,10 +1011,15 @@ void Shell::Init(
     rgb_keyboard_manager_ = std::make_unique<RgbKeyboardManager>();
   }
 
-  // Observes the tablet mode controller and adds a notification to the message
-  // center, so must be constructed after both.
-  if (features::IsSnoopingProtectionEnabled()) {
+  // Observes the tablet mode controller if any hps feature is enabled.
+  if (features::IsSnoopingProtectionEnabled() ||
+      features::IsQuickDimEnabled()) {
     hps_orientation_controller_ = std::make_unique<HpsOrientationController>();
+  }
+
+  // Construct HpsNotifyController, must be constructed after
+  // HpsOrientationController.
+  if (features::IsSnoopingProtectionEnabled()) {
     hps_notify_controller_ = std::make_unique<HpsNotifyController>();
   }
 

ash/shell.h

@@ -781,6 +781,7 @@ class ASH_EXPORT Shell : public SessionObserver,
   std::unique_ptr<FloatController> float_controller_;
   std::unique_ptr<GeolocationController> geolocation_controller_;
   std::unique_ptr<HoldingSpaceController> holding_space_controller_;
+  std::unique_ptr<PowerPrefs> power_prefs_;
   std::unique_ptr<HpsNotifyController> hps_notify_controller_;
   std::unique_ptr<HpsOrientationController> hps_orientation_controller_;
   std::unique_ptr<ImeControllerImpl> ime_controller_;
@@ -892,7 +893,6 @@ class ASH_EXPORT Shell : public SessionObserver,
   std::unique_ptr<PeripheralBatteryListener> peripheral_battery_listener_;
   std::unique_ptr<PeripheralBatteryNotifier> peripheral_battery_notifier_;
   std::unique_ptr<PowerEventObserver> power_event_observer_;
-  std::unique_ptr<PowerPrefs> power_prefs_;
   std::unique_ptr<ui::UserActivityPowerManagerNotifier> user_activity_notifier_;
   std::unique_ptr<VideoActivityNotifier> video_activity_notifier_;
   std::unique_ptr<StickyKeysController> sticky_keys_controller_;

ash/system/power/hps_sense_controller.cc

@@ -4,6 +4,7 @@
 
 #include "ash/system/power/hps_sense_controller.h"
 
+#include "ash/shell.h"
 #include "base/bind.h"
 #include "chromeos/components/hps/hps_configuration.h"
 #include "chromeos/dbus/hps/hps_dbus_client.h"
@@ -29,37 +30,37 @@ void DisableHpsSenseViaDBus() {
 
 HpsSenseController::HpsSenseController() {
   hps_observation_.Observe(chromeos::HpsDBusClient::Get());
+
   chromeos::HpsDBusClient::Get()->WaitForServiceToBeAvailable(
       base::BindOnce(&HpsSenseController::OnHpsServiceAvailable,
                      weak_ptr_factory_.GetWeakPtr()));
+
+  // Orientation controller is instantiated before us in the shell.
+  HpsOrientationController* orientation_controller =
+      Shell::Get()->hps_orientation_controller();
+  suitable_for_hps_ = orientation_controller->IsOrientationSuitable();
+  orientation_observation_.Observe(orientation_controller);
 }
 
 HpsSenseController::~HpsSenseController() {
   hps_observation_.Reset();
+  orientation_observation_.Reset();
 }
 
 void HpsSenseController::EnableHpsSense() {
-  // Only enable if HpsSense is not enabled yet.
-  if (want_hps_sense_)
-    return;
   want_hps_sense_ = true;
-
-  // If hps_service is available then EnableHpsSense; otherwise, it will be
-  // enabled when the service becomes available.
-  if (service_available_)
-    EnableHpsSenseViaDBus();
+  ReconfigViaDbus();
 }
 
 void HpsSenseController::DisableHpsSense() {
-  // Only disable if HpsSense is enabled currently.
-  if (!want_hps_sense_)
-    return;
   want_hps_sense_ = false;
+  ReconfigViaDbus();
+}
 
-  // If hps_service is available then DisableHpsSense; otherwise, it will be
-  // disabled when the service becomes available.
-  if (service_available_)
-    DisableHpsSenseViaDBus();
+// HpsOrientationObserver:
+void HpsSenseController::OnOrientationChanged(bool suitable_for_hps) {
+  suitable_for_hps_ = suitable_for_hps;
+  ReconfigViaDbus();
 }
 
 void HpsSenseController::OnHpsSenseChanged(hps::HpsResult state) {}
@@ -68,29 +69,46 @@ void HpsSenseController::OnHpsNotifyChanged(hps::HpsResult state) {}
 
 void HpsSenseController::OnRestart() {
   service_available_ = true;
-
-  // HpsDBusService just restarted, only need to send enabling signal.
-  if (want_hps_sense_)
-    EnableHpsSenseViaDBus();
+  ReconfigViaDbus();
 }
 
 void HpsSenseController::OnShutdown() {
   // HpsDBusService just stopped.
   service_available_ = false;
+  configured_state_ = ConfiguredHpsSenseState::kDisabled;
 }
 
-void HpsSenseController::OnHpsServiceAvailable(
-    const bool service_is_available) {
-  if (!service_is_available)
+void HpsSenseController::OnHpsServiceAvailable(const bool service_available) {
+  service_available_ = service_available;
+  ReconfigViaDbus();
+}
+
+void HpsSenseController::ReconfigViaDbus() {
+  if (!service_available_)
     return;
-  service_available_ = true;
 
-  // Always disable first, just in case the service was left enabled from
-  // previous chrome session.
-  DisableHpsSenseViaDBus();
+  // When chrome starts, it does not know the current configured_state_, because
+  // it could be left enabled from previous chrome session, disable it so that
+  // the new configuration can apply.
+  if (configured_state_ == ConfiguredHpsSenseState::kUnknown) {
+    DisableHpsSenseViaDBus();
+    configured_state_ = ConfiguredHpsSenseState::kDisabled;
+  }
+
+  // Wanted state should be either kEnabled or kDisabled.
+  const ConfiguredHpsSenseState wanted_state =
+      want_hps_sense_ && suitable_for_hps_ ? ConfiguredHpsSenseState::kEnabled
+                                           : ConfiguredHpsSenseState::kDisabled;
 
-  if (want_hps_sense_)
+  // Return if already configured to the wanted state.
+  if (wanted_state == configured_state_)
+    return;
+
+  if (wanted_state == ConfiguredHpsSenseState::kEnabled) {
     EnableHpsSenseViaDBus();
+  } else {
+    DisableHpsSenseViaDBus();
+  }
+  configured_state_ = wanted_state;
 }
-
 }  // namespace ash

ash/system/power/hps_sense_controller.h

@@ -6,6 +6,7 @@
 #define ASH_SYSTEM_POWER_HPS_SENSE_CONTROLLER_H_
 
 #include "ash/ash_export.h"
+#include "ash/system/hps/hps_orientation_controller.h"
 #include "base/memory/weak_ptr.h"
 #include "base/scoped_observation.h"
 #include "chromeos/dbus/hps/hps_dbus_client.h"
@@ -14,9 +15,20 @@
 namespace ash {
 
 // Helper class for chromeos::HpsDBusClient, responsible for enabling/disabling
-// HPS via the client and is responsible for maintaining state between restarts.
-class ASH_EXPORT HpsSenseController : public chromeos::HpsDBusClient::Observer {
+// the DBus service via the client and is responsible for maintaining state
+// between restarts.
+class ASH_EXPORT HpsSenseController : public HpsOrientationController::Observer,
+                                      chromeos::HpsDBusClient::Observer {
  public:
+  // The state of HpsSense inside HpsDbusService that is configured. It is set
+  // as kUnknown in this class on initialization. And is set to either kEnable
+  // or kDisable when EnableHpsSense() or DisableHpsSense() is called.
+  enum class ConfiguredHpsSenseState {
+    kUnknown,
+    kEnabled,
+    kDisabled,
+  };
+
   HpsSenseController();
   HpsSenseController(const HpsSenseController&) = delete;
   HpsSenseController& operator=(const HpsSenseController&) = delete;
@@ -30,6 +42,9 @@ class ASH_EXPORT HpsSenseController : public chromeos::HpsDBusClient::Observer {
   // enabled.
   void DisableHpsSense();
 
+  // HpsOrientationObserver:
+  void OnOrientationChanged(bool suitable_for_hps) override;
+
   // chromeos::HpsDBusClient::Observer:
   void OnHpsSenseChanged(hps::HpsResult state) override;
   void OnHpsNotifyChanged(hps::HpsResult state) override;
@@ -39,7 +54,10 @@ class ASH_EXPORT HpsSenseController : public chromeos::HpsDBusClient::Observer {
 
  private:
   // Called when the Hps Service is available.
-  void OnHpsServiceAvailable(bool service_is_available);
+  void OnHpsServiceAvailable(bool service_available);
+
+  // May disable/enable hps_sense based on current state.
+  void ReconfigViaDbus();
 
   // Indicates whether the hps service is available; it is set inside
   // OnHpsServiceAvailable and set to false OnShutdown.
@@ -48,9 +66,19 @@ class ASH_EXPORT HpsSenseController : public chromeos::HpsDBusClient::Observer {
   // Records requested hps sense enable state from client.
   bool want_hps_sense_ = false;
 
+  // Whether the device is in physical orientation where our models are
+  // accurate.
+  bool suitable_for_hps_ = false;
+
+  // Current configured state of HpsSense.
+  ConfiguredHpsSenseState configured_state_ = ConfiguredHpsSenseState::kUnknown;
+
   base::ScopedObservation<chromeos::HpsDBusClient,
                           chromeos::HpsDBusClient::Observer>
       hps_observation_{this};
+  base::ScopedObservation<HpsOrientationController,
+                          HpsOrientationController::Observer>
+      orientation_observation_{this};
   base::WeakPtrFactory<HpsSenseController> weak_ptr_factory_{this};
 };
 

ash/system/power/hps_sense_controller_unittest.cc

@@ -6,25 +6,39 @@
 
 #include <memory>
 
+#include "ash/constants/ash_features.h"
+#include "ash/constants/ash_switches.h"
+#include "ash/test/ash_test_base.h"
 #include "base/run_loop.h"
-#include "base/test/task_environment.h"
+#include "base/test/scoped_command_line.h"
+#include "base/test/scoped_feature_list.h"
 #include "chromeos/dbus/hps/fake_hps_dbus_client.h"
 #include "chromeos/dbus/hps/hps_dbus_client.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace ash {
 
-class HpsSenseControllerTest : public testing::Test {
+class HpsSenseControllerTest : public AshTestBase {
  public:
   void SetUp() override {
+    // We need to enable kQuickDim to construct HpsOrientationController.
+    scoped_feature_list_.InitAndEnableFeature(features::kQuickDim);
+    base::CommandLine::ForCurrentProcess()->AppendSwitch(switches::kHasHps);
+
+    // Initialize FakeHpsDBusClient.
     chromeos::HpsDBusClient::InitializeFake();
     hps_client_ = chromeos::FakeHpsDBusClient::Get();
     hps_client_->Reset();
+
+    AshTestBase::SetUp();
   }
 
  protected:
   chromeos::FakeHpsDBusClient* hps_client_ = nullptr;
-  base::test::SingleThreadTaskEnvironment task_environment_;
+
+ private:
+  base::test::ScopedFeatureList scoped_feature_list_;
+  base::test::ScopedCommandLine scoped_command_line_;
 };
 
 // EnableHpsSense should be skipped if HpsService is not available.
@@ -122,7 +136,7 @@ TEST_F(HpsSenseControllerTest, DisableHpsSenseOnlyCalledOnceOnTwoCalls) {
   EXPECT_EQ(hps_client_->enable_hps_sense_count(), 0);
 }
 
-// EnableHpsSense should be called on restart if HpsSense is enabled currently.
+// No dbus call should be sent on restart if HpsSense is currently disabled.
 TEST_F(HpsSenseControllerTest, NoDbusCallsOnRestartIfHpsSenseDisabled) {
   hps_client_->set_hps_service_is_available(true);
   auto hps_sense_controller = std::make_unique<HpsSenseController>();
@@ -131,6 +145,7 @@ TEST_F(HpsSenseControllerTest, NoDbusCallsOnRestartIfHpsSenseDisabled) {
   EXPECT_EQ(hps_client_->enable_hps_sense_count(), 0);
 
   // HpsSense is disabled; Restart will not send dbus calls.
+  hps_client_->Shutdown();
   hps_client_->Restart();
   base::RunLoop().RunUntilIdle();
   EXPECT_EQ(hps_client_->disable_hps_sense_count(), 1);
@@ -148,6 +163,7 @@ TEST_F(HpsSenseControllerTest, EnableHpsSenseOnRestartIfHpsSenseWasEnabled) {
   EXPECT_EQ(hps_client_->enable_hps_sense_count(), 1);
 
   // HpsSense is enabled; Restart will call EnableHpsSense.
+  hps_client_->Shutdown();
   hps_client_->Restart();
   base::RunLoop().RunUntilIdle();
   EXPECT_EQ(hps_client_->disable_hps_sense_count(), 1);
@@ -170,4 +186,40 @@ TEST_F(HpsSenseControllerTest, AlwaysCallDisableOnServiceAvailable) {
   EXPECT_EQ(hps_client_->enable_hps_sense_count(), 1);
 }
 
+// Confirm that enable_hps_sense is only called when both EnableHpsSense and
+// OnOrientationChanged(true) is set.
+TEST_F(HpsSenseControllerTest, OreientationChanging) {
+  hps_client_->set_hps_service_is_available(true);
+  auto hps_sense_controller = std::make_unique<HpsSenseController>();
+  hps_sense_controller->EnableHpsSense();
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(hps_client_->disable_hps_sense_count(), 1);
+  EXPECT_EQ(hps_client_->enable_hps_sense_count(), 1);
+  hps_client_->Reset();
+
+  // Orientation changed, HpsSense should be disabled.
+  hps_sense_controller->OnOrientationChanged(false);
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(hps_client_->disable_hps_sense_count(), 1);
+  EXPECT_EQ(hps_client_->enable_hps_sense_count(), 0);
+  hps_client_->Reset();
+
+  // Calling enable/disable will not sent any dbus call while OrientationChanged
+  // to be false.
+  hps_sense_controller->DisableHpsSense();
+  base::RunLoop().RunUntilIdle();
+  hps_sense_controller->OnOrientationChanged(false);
+  base::RunLoop().RunUntilIdle();
+  hps_sense_controller->EnableHpsSense();
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(hps_client_->disable_hps_sense_count(), 0);
+  EXPECT_EQ(hps_client_->enable_hps_sense_count(), 0);
+
+  // Changing oritiantation will trigger enabling if EnableHpsSense was set.
+  hps_sense_controller->OnOrientationChanged(true);
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(hps_client_->disable_hps_sense_count(), 0);
+  EXPECT_EQ(hps_client_->enable_hps_sense_count(), 1);
+}
+
 }  // namespace ash

chromeos/dbus/hps/fake_hps_dbus_client.cc

@@ -82,6 +82,12 @@ void FakeHpsDBusClient::Restart() {
   }
 }
 
+void FakeHpsDBusClient::Shutdown() {
+  for (auto& observer : observers_) {
+    observer.OnShutdown();
+  }
+}
+
 void FakeHpsDBusClient::Reset() {
   hps_notify_result_.reset();
   hps_notify_count_ = 0;

chromeos/dbus/hps/fake_hps_dbus_client.h

@@ -64,6 +64,9 @@ class COMPONENT_EXPORT(HPS) FakeHpsDBusClient : public HpsDBusClient {
   // Simulte HpsService restart.
   void Restart();
 
+  // Simulte HpsService shutdown.
+  void Shutdown();
+
   // Resets all parameters; used in unittests.
   void Reset();