TabletModeController refactoring III.

Changes in this CL:
- Remove have_seen_accelerometer_data_ variable. As this variable is not
  set if the device doesn't have kAshEnableTabletMode flag.
- Only observe the display/lid/input device events if the ui mode is
  allowed to be changed.

No functionality is changed in this CL.

Bug: 887042
Change-Id: Ic42d4cc0c25667e7aa67efcf26f65032082db126
Reviewed-on: https://chromium-review.googlesource.com/1244289Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Commit-Queue: Xiaoqian Dai <xdai@chromium.org>
Cr-Commit-Position: refs/heads/master@{#594407}

ash/public/cpp/ash_switches.cc

@@ -49,9 +49,12 @@ const char kAshEnableMagnifierKeyScroller[] =
 const char kAshEnablePaletteOnAllDisplays[] =
     "ash-enable-palette-on-all-displays";
 
-// Enables the observation of accelerometer events to enter tablet
-// mode.  The flag is "enable-touchview" not "enable-tabletmode" as this
-// is used to enable tablet mode on convertible devices.
+// If the flag is present, it indicates 1) the device has accelerometer and 2)
+// the device is a convertible device or a tablet device (thus is capable of
+// entering tablet mode). If this flag is not set, then the device is not
+// capable of entering tablet mode. For example, Samus has accelerometer, but
+// is not a covertible or tablet, thus doesn't have this flag set, thus can't
+// enter tablet mode.
 const char kAshEnableTabletMode[] = "enable-touchview";
 
 // Enable the wayland server.

ash/wm/tablet_mode/tablet_mode_controller.cc

@@ -93,7 +93,11 @@ bool IsAngleBetweenAccelerometerReadingsStable(
                  .Length()) <= kNoisyMagnitudeDeviation;
 }
 
-bool IsEnabled() {
+// Returns true if the device is capable of entering tablet mode. This only
+// returns true if the device has kAshEnableTabletMode flag, which means 1) the
+// device has accelerometer and 2) the device is a convertible or a tablet.
+// A device without this flag is not capable of entering tablet mode.
+bool IsTabletModeCapable() {
   return base::CommandLine::ForCurrentProcess()->HasSwitch(
       switches::kAshEnableTabletMode);
 }
@@ -130,26 +134,6 @@ TabletModeController::TabletModeController()
       scoped_session_observer_(this),
       weak_factory_(this) {
   Shell::Get()->AddShellObserver(this);
-  base::RecordAction(base::UserMetricsAction("Touchview_Initially_Disabled"));
-
-  // TODO(jonross): Do not create TabletModeController if the flag is
-  // unavailable. This will require refactoring
-  // IsTabletModeWindowManagerEnabled to check for the existence of the
-  // controller.
-  if (IsEnabled()) {
-    Shell::Get()->window_tree_host_manager()->AddObserver(this);
-    chromeos::AccelerometerReader::GetInstance()->AddObserver(this);
-    ui::InputDeviceManager::GetInstance()->AddObserver(this);
-    bluetooth_devices_observer_ = std::make_unique<BluetoothDevicesObserver>(
-        base::BindRepeating(&TabletModeController::UpdateBluetoothDevice,
-                            base::Unretained(this)));
-  }
-  chromeos::PowerManagerClient* power_manager_client =
-      chromeos::DBusThreadManager::Get()->GetPowerManagerClient();
-  power_manager_client->AddObserver(this);
-  power_manager_client->GetSwitchStates(base::BindOnce(
-      &TabletModeController::OnGetSwitchStates, weak_factory_.GetWeakPtr()));
-
   TabletMode::SetCallback(base::BindRepeating(
       &TabletModeController::IsTabletModeWindowManagerEnabled,
       base::Unretained(this)));
@@ -158,13 +142,13 @@ TabletModeController::TabletModeController()
 TabletModeController::~TabletModeController() {
   Shell::Get()->RemoveShellObserver(this);
 
-  if (IsEnabled()) {
+  if (AllowUiModeChange() && IsTabletModeCapable()) {
     Shell::Get()->window_tree_host_manager()->RemoveObserver(this);
     chromeos::AccelerometerReader::GetInstance()->RemoveObserver(this);
     ui::InputDeviceManager::GetInstance()->RemoveObserver(this);
+    chromeos::DBusThreadManager::Get()->GetPowerManagerClient()->RemoveObserver(
+        this);
   }
-  chromeos::DBusThreadManager::Get()->GetPowerManagerClient()->RemoveObserver(
-      this);
 
   TabletMode::SetCallback(TabletMode::TabletModeCallback());
 }
@@ -209,7 +193,7 @@ void TabletModeController::EnableTabletModeWindowManager(bool should_enable) {
 }
 
 bool TabletModeController::IsTabletModeWindowManagerEnabled() const {
-  return tablet_mode_window_manager_.get() != NULL;
+  return tablet_mode_window_manager_.get() != nullptr;
 }
 
 void TabletModeController::AddWindow(aura::Window* window) {
@@ -259,8 +243,26 @@ bool TabletModeController::TriggerRecordLidAngleTimerForTesting() {
 
 void TabletModeController::OnShellInitialized() {
   force_ui_mode_ = GetTabletMode();
-  if (force_ui_mode_ == UiMode::kTabletMode)
-    AttemptEnterTabletMode();
+  if (!AllowUiModeChange()) {
+    // |force_ui_mode_| should have the top priority. If the user sets the
+    // |force_ui_mode_| to kTabletMode or kClamshell, always respect the user's
+    // choice.
+    EnableTabletModeWindowManager(force_ui_mode_ == UiMode::kTabletMode);
+  } else if (IsTabletModeCapable()) {
+    base::RecordAction(base::UserMetricsAction("Touchview_Initially_Disabled"));
+
+    Shell::Get()->window_tree_host_manager()->AddObserver(this);
+    chromeos::AccelerometerReader::GetInstance()->AddObserver(this);
+    ui::InputDeviceManager::GetInstance()->AddObserver(this);
+
+    chromeos::PowerManagerClient* power_manager_client =
+        chromeos::DBusThreadManager::Get()->GetPowerManagerClient();
+    power_manager_client->AddObserver(this);
+    power_manager_client->GetSwitchStates(base::BindOnce(
+        &TabletModeController::OnGetSwitchStates, weak_factory_.GetWeakPtr()));
+  }
+  // Otherwise the device is not capable of entering tablet mode, do nothing to
+  // keep it in clamshell mode.
 }
 
 void TabletModeController::OnDisplayConfigurationChanged() {
@@ -283,7 +285,7 @@ void TabletModeController::OnChromeTerminating() {
   // metrics based on whether TabletMode mode is currently active.
   RecordTabletModeUsageInterval(CurrentTabletModeIntervalType());
 
-  if (CanEnterTabletMode()) {
+  if (IsTabletModeCapable()) {
     UMA_HISTOGRAM_CUSTOM_COUNTS("Ash.TouchView.TouchViewActiveTotal",
                                 total_tablet_mode_time_.InMinutes(), 1,
                                 base::TimeDelta::FromDays(7).InMinutes(), 50);
@@ -305,7 +307,6 @@ void TabletModeController::OnAccelerometerUpdated(
   if (!AllowUiModeChange())
     return;
 
-  have_seen_accelerometer_data_ = true;
   can_detect_lid_angle_ =
       update->has(chromeos::ACCELEROMETER_SOURCE_SCREEN) &&
       update->has(chromeos::ACCELEROMETER_SOURCE_ATTACHED_KEYBOARD);
@@ -488,13 +489,6 @@ bool TabletModeController::CanUseUnstableLidAngle() const {
   return elapsed_time >= kUnstableLidAngleDuration;
 }
 
-bool TabletModeController::CanEnterTabletMode() {
-  // If we have ever seen accelerometer data, then HandleHingeRotation may
-  // trigger tablet mode at some point in the future.
-  // All TabletMode-enabled devices can enter tablet mode.
-  return have_seen_accelerometer_data_ || IsEnabled();
-}
-
 void TabletModeController::AttemptEnterTabletMode() {
   if (IsTabletModeWindowManagerEnabled() || has_external_mouse_) {
     UpdateInternalMouseAndKeyboardEventBlocker();
@@ -515,7 +509,7 @@ void TabletModeController::AttemptLeaveTabletMode() {
 
 void TabletModeController::RecordTabletModeUsageInterval(
     TabletModeIntervalType type) {
-  if (!CanEnterTabletMode())
+  if (!IsTabletModeCapable())
     return;
 
   base::Time current_time = base::Time::Now();

ash/wm/tablet_mode/tablet_mode_controller.h

@@ -82,6 +82,8 @@ class ASH_EXPORT TabletModeController
   // unittests; the event blocker prevents keyboard input when running ChromeOS
   // on linux. http://crbug.com/362881
   // Turn the always tablet mode window manager on or off.
+  // TODO(xdai): Make it a private function. This function is not supposed to be
+  // called by an external caller except for tests.
   void EnableTabletModeWindowManager(bool should_enable);
 
   // Test if the TabletModeWindowManager is enabled or not.
@@ -162,11 +164,6 @@ class ASH_EXPORT TabletModeController
   // a certain range of time before using unstable angle.
   bool CanUseUnstableLidAngle() const;
 
-  // True if it is possible to enter tablet mode in the current
-  // configuration. If this returns false, it should never be the case that
-  // tablet mode becomes enabled.
-  bool CanEnterTabletMode();
-
   // Attempts to enter tablet mode and update the internal keyboard and
   // touchpad.
   void AttemptEnterTabletMode();
@@ -223,9 +220,6 @@ class ASH_EXPORT TabletModeController
   // internal keyboard and touchpad.
   std::unique_ptr<ScopedDisableInternalMouseAndKeyboard> event_blocker_;
 
-  // Whether we have ever seen accelerometer data.
-  bool have_seen_accelerometer_data_ = false;
-
   // Whether the lid angle can be detected. If it's true, the device is a
   // convertible device (both screen acclerometer and keyboard acclerometer are
   // available, thus lid angle is detectable). And if it's false, the device is

ash/wm/tablet_mode/tablet_mode_controller_unittest.cc

@@ -636,59 +636,13 @@ TEST_F(TabletModeControllerTest, InitializedWhileTabletModeSwitchOn) {
   TabletMode::SetCallback({});
 
   TabletModeController controller;
+  controller.OnShellInitialized();
   EXPECT_FALSE(controller.IsTabletModeWindowManagerEnabled());
   // PowerManagerClient callback is a posted task.
   base::RunLoop().RunUntilIdle();
   EXPECT_TRUE(controller.IsTabletModeWindowManagerEnabled());
 }
 
-// Verify when the force touch view mode flag is turned on, tablet mode is on
-// initially, and opening the lid to less than 180 degress or setting tablet
-// mode to off will not turn off tablet mode.
-TEST_F(TabletModeControllerTest, ForceTabletModeTest) {
-  base::CommandLine::ForCurrentProcess()->AppendSwitchASCII(
-      switches::kAshUiMode, switches::kAshUiModeTablet);
-  tablet_mode_controller()->OnShellInitialized();
-  EXPECT_EQ(TabletModeController::UiMode::kTabletMode, forced_ui_mode());
-  EXPECT_TRUE(IsTabletModeStarted());
-  EXPECT_TRUE(AreEventsBlocked());
-
-  OpenLidToAngle(30.0f);
-  EXPECT_TRUE(IsTabletModeStarted());
-  EXPECT_TRUE(AreEventsBlocked());
-
-  SetTabletMode(false);
-  EXPECT_TRUE(IsTabletModeStarted());
-  EXPECT_TRUE(AreEventsBlocked());
-
-  // Tests that attaching a external mouse will not change the mode.
-  ws::InputDeviceClientTestApi().SetMouseDevices(
-      {ui::InputDevice(3, ui::InputDeviceType::INPUT_DEVICE_USB, "mouse")});
-  base::RunLoop().RunUntilIdle();
-  EXPECT_TRUE(IsTabletModeStarted());
-  EXPECT_TRUE(AreEventsBlocked());
-}
-
-// Tests that when the force touch view mode flag is set to clamshell, clamshell
-// mode is on initially, and cannot be changed by lid angle or manually entering
-// tablet mode.
-TEST_F(TabletModeControllerTest, ForceClamshellModeTest) {
-  base::CommandLine::ForCurrentProcess()->AppendSwitchASCII(
-      switches::kAshUiMode, switches::kAshUiModeClamshell);
-  tablet_mode_controller()->OnShellInitialized();
-  EXPECT_EQ(TabletModeController::UiMode::kClamshell, forced_ui_mode());
-  EXPECT_FALSE(IsTabletModeStarted());
-  EXPECT_FALSE(AreEventsBlocked());
-
-  OpenLidToAngle(200.0f);
-  EXPECT_FALSE(IsTabletModeStarted());
-  EXPECT_FALSE(AreEventsBlocked());
-
-  SetTabletMode(true);
-  EXPECT_FALSE(IsTabletModeStarted());
-  EXPECT_FALSE(AreEventsBlocked());
-}
-
 TEST_F(TabletModeControllerTest, RestoreAfterExit) {
   UpdateDisplay("1000x600");
   std::unique_ptr<aura::Window> w1(
@@ -829,4 +783,79 @@ TEST_F(TabletModeControllerTest, ExternalMouseInLaptopMode) {
   EXPECT_FALSE(AreEventsBlocked());
 }
 
+class TabletModeControllerForceTabletModeTest
+    : public TabletModeControllerTest {
+ public:
+  TabletModeControllerForceTabletModeTest() = default;
+  ~TabletModeControllerForceTabletModeTest() override = default;
+
+  // AshTestBase:
+  void SetUp() override {
+    base::CommandLine::ForCurrentProcess()->AppendSwitchASCII(
+        switches::kAshUiMode, switches::kAshUiModeTablet);
+    TabletModeControllerTest::SetUp();
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(TabletModeControllerForceTabletModeTest);
+};
+
+// Verify when the force touch view mode flag is turned on, tablet mode is on
+// initially, and opening the lid to less than 180 degress or setting tablet
+// mode to off will not turn off tablet mode.
+TEST_F(TabletModeControllerForceTabletModeTest, ForceTabletModeTest) {
+  EXPECT_EQ(TabletModeController::UiMode::kTabletMode, forced_ui_mode());
+  EXPECT_TRUE(IsTabletModeStarted());
+  EXPECT_TRUE(AreEventsBlocked());
+
+  OpenLidToAngle(30.0f);
+  EXPECT_TRUE(IsTabletModeStarted());
+  EXPECT_TRUE(AreEventsBlocked());
+
+  SetTabletMode(false);
+  EXPECT_TRUE(IsTabletModeStarted());
+  EXPECT_TRUE(AreEventsBlocked());
+
+  // Tests that attaching a external mouse will not change the mode.
+  ws::InputDeviceClientTestApi().SetMouseDevices(
+      {ui::InputDevice(3, ui::InputDeviceType::INPUT_DEVICE_USB, "mouse")});
+  base::RunLoop().RunUntilIdle();
+  EXPECT_TRUE(IsTabletModeStarted());
+  EXPECT_TRUE(AreEventsBlocked());
+}
+
+class TabletModeControllerForceClamshellModeTest
+    : public TabletModeControllerTest {
+ public:
+  TabletModeControllerForceClamshellModeTest() = default;
+  ~TabletModeControllerForceClamshellModeTest() override = default;
+
+  // AshTestBase:
+  void SetUp() override {
+    base::CommandLine::ForCurrentProcess()->AppendSwitchASCII(
+        switches::kAshUiMode, switches::kAshUiModeClamshell);
+    TabletModeControllerTest::SetUp();
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(TabletModeControllerForceClamshellModeTest);
+};
+
+// Tests that when the force touch view mode flag is set to clamshell, clamshell
+// mode is on initially, and cannot be changed by lid angle or manually entering
+// tablet mode.
+TEST_F(TabletModeControllerForceClamshellModeTest, ForceClamshellModeTest) {
+  EXPECT_EQ(TabletModeController::UiMode::kClamshell, forced_ui_mode());
+  EXPECT_FALSE(IsTabletModeStarted());
+  EXPECT_FALSE(AreEventsBlocked());
+
+  OpenLidToAngle(200.0f);
+  EXPECT_FALSE(IsTabletModeStarted());
+  EXPECT_FALSE(AreEventsBlocked());
+
+  SetTabletMode(true);
+  EXPECT_FALSE(IsTabletModeStarted());
+  EXPECT_FALSE(AreEventsBlocked());
+}
+
 }  // namespace ash