Prevent entering maximize mode when the lid is closed or has recently opened.

ash/wm/maximize_mode/maximize_mode_controller.cc

 // Copyright 2014 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 "ash/wm/maximize_mode/maximize_mode_controller.h"
 
 #include "ash/accelerators/accelerator_controller.h"
 #include "ash/accelerators/accelerator_table.h"
 #include "ash/accelerometer/accelerometer_controller.h"
 #include "ash/ash_switches.h"
 #include "ash/display/display_manager.h"
 #include "ash/shell.h"
 #include "ash/wm/maximize_mode/maximize_mode_window_manager.h"
 #include "ash/wm/maximize_mode/scoped_disable_internal_mouse_and_keyboard.h"
 #include "base/auto_reset.h"
 #include "base/command_line.h"
 #include "base/metrics/histogram.h"
+#include "base/time/default_tick_clock.h"
+#include "base/time/tick_clock.h"
 #include "ui/base/accelerators/accelerator.h"
 #include "ui/events/event.h"
 #include "ui/events/event_handler.h"
 #include "ui/events/keycodes/keyboard_codes.h"
 #include "ui/gfx/vector3d_f.h"
 
 #if defined(USE_X11)
 #include "ash/wm/maximize_mode/scoped_disable_internal_mouse_and_keyboard_x11.h"
 #endif
 
+#if defined(OS_CHROMEOS)
+#include "chromeos/dbus/dbus_thread_manager.h"
+#endif  // OS_CHROMEOS
+
 namespace ash {
 
 namespace {
 
 // The hinge angle at which to enter maximize mode.
 const float kEnterMaximizeModeAngle = 200.0f;
 
 // The angle at which to exit maximize mode, this is specifically less than the
 // angle to enter maximize mode to prevent rapid toggling when near the angle.
 const float kExitMaximizeModeAngle = 160.0f;
 
-// When the lid is fully open 360 degrees, the accelerometer readings can
-// occasionally appear as though the lid is almost closed. If the lid appears
-// near closed but the device is on we assume it is an erroneous reading from
-// it being open 360 degrees.
-const float kFullyOpenAngleErrorTolerance = 20.0f;
+// Defines a range for which accelerometer readings are considered accurate.
+// When the lid is near open (or near closed) the acceleromter readings may be
+// inaccurate and a lid that is fully open may appear to be near closed (and
+// vice versa).
+const float kMinStableAngle = 20.0f;
+const float kMaxStableAngle = 340.0f;
+
+// The time duration to consider the lid to be recently opened.
+// This is used to prevent entering maximize mode if an erroneous accelerometer
+// reading makes the lid appear to be fully open when the user is opening the
+// lid from a closed position.
+const base::TimeDelta kLidRecentlyOpenedTolerance =

[flackr, 2014/07/29 16:41:45]

how about kLidRecentlyOpenedDuration?

[bruthig, 2014/07/31 16:10:48]

Done.

+  base::TimeDelta::FromSeconds(2);

[flackr, 2014/07/29 16:41:44]

nit: indent 4.

[bruthig, 2014/07/31 16:10:49]

Done.

 
 // When the device approaches vertical orientation (i.e. portrait orientation)
 // the accelerometers for the base and lid approach the same values (i.e.
 // gravity pointing in the direction of the hinge). When this happens we cannot
 // compute the hinge angle reliably and must turn ignore accelerometer readings.
 // This is the minimum acceleration perpendicular to the hinge under which to
 // detect hinge angle.
 const float kHingeAngleDetectionThreshold = 0.25f;
 
 // The maximum deviation from the acceleration expected due to gravity under
@@ skipping 12 matching lines @@
 
 // The minimum acceleration in a direction required to trigger screen rotation.
 // This prevents rapid toggling of rotation when the device is near flat and
 // there is very little screen aligned force on it. The value is effectively the
 // sine of the rise angle required, with the current value requiring at least a
 // 25 degree rise.
 const float kMinimumAccelerationScreenRotation = 0.42f;
 
 const float kRadiansToDegrees = 180.0f / 3.14159265f;
 
+// Returns true if the angle is considered a stable accurate reading.
+bool IsAngleStable(float angle) {

[flackr, 2014/07/29 16:41:45]

I feel like this should be defined as a bool where it is used since we only call
it with the current angle.

[bruthig, 2014/07/31 16:10:49]

Done.

+  return angle > kMinStableAngle && angle < kMaxStableAngle;
+}
+
 // Returns the angle between |base| and |other| in degrees.
 float AngleBetweenVectorsInDegrees(const gfx::Vector3dF& base,
                                  const gfx::Vector3dF& other) {
   return acos(gfx::DotProduct(base, other) /
               base.Length() / other.Length()) * kRadiansToDegrees;
 }
 
 // Returns the clockwise angle between |base| and |other| where |normal| is the
 // normal of the virtual surface to measure clockwise according to.
 float ClockwiseAngleBetweenVectorsInDegrees(const gfx::Vector3dF& base,
@@ skipping 51 matching lines @@
 
 #endif  // OS_CHROMEOS
 
 }  // namespace
 
 MaximizeModeController::MaximizeModeController()
     : rotation_locked_(false),
       have_seen_accelerometer_data_(false),
       in_set_screen_rotation_(false),
       user_rotation_(gfx::Display::ROTATE_0),
-      last_touchview_transition_time_(base::Time::Now()) {
+      last_touchview_transition_time_(base::Time::Now()),
+      last_lid_open_time_(),

[flackr, 2014/07/29 16:41:44]

Default constructor is automatically called, no need to explicitly call in
initializer list.

[bruthig, 2014/07/31 16:10:49]

Done.

+      tick_clock_(new base::DefaultTickClock()),
+      lid_is_closed_(false) {
   Shell::GetInstance()->accelerometer_controller()->AddObserver(this);
   Shell::GetInstance()->AddShellObserver(this);
+#if defined(OS_CHROMEOS)
+  chromeos::DBusThreadManager::Get()->
+    GetPowerManagerClient()->AddObserver(this);

[flackr, 2014/07/29 16:41:44]

nit: indent 4.

[bruthig, 2014/07/31 16:10:48]

Done.

+#endif  // OS_CHROMEOS
 }
 
 MaximizeModeController::~MaximizeModeController() {
   Shell::GetInstance()->RemoveShellObserver(this);
   Shell::GetInstance()->accelerometer_controller()->RemoveObserver(this);
+#if defined(OS_CHROMEOS)
+  chromeos::DBusThreadManager::Get()->
+    GetPowerManagerClient()->RemoveObserver(this);

[flackr, 2014/07/29 16:41:44]

nit: indent 4.

[bruthig, 2014/07/31 16:10:49]

Done.

+#endif  // OS_CHROMEOS
 }
 
 void MaximizeModeController::SetRotationLocked(bool rotation_locked) {
   if (rotation_locked_ == rotation_locked)
     return;
   rotation_locked_ = rotation_locked;
   FOR_EACH_OBSERVER(Observer, observers_,
                     OnRotationLockChanged(rotation_locked_));
 }
 
@@ skipping 67 matching lines @@
   if (user_rotation != current_rotation_) {
     // A user may change other display configuration settings. When the user
     // does change the rotation setting, then lock rotation to prevent the
     // accelerometer from erasing their change.
     SetRotationLocked(true);
     user_rotation_ = user_rotation;
     current_rotation_ = user_rotation;
   }
 }
 
+#if defined(OS_CHROMEOS)
+void MaximizeModeController::LidEventReceived(bool open,
+                                              const base::TimeTicks& time) {
+  if (open)
+    last_lid_open_time_ = time;
+  lid_is_closed_ = !open;
+  LeaveMaximizeMode();
+}
+
+void MaximizeModeController::SuspendImminent() {
+  RecordTouchViewStateTransition();
+}
+
+void MaximizeModeController::SuspendDone(
+    const base::TimeDelta& sleep_duration) {
+  last_touchview_transition_time_ = base::Time::Now();
+  // A lid open event won't always occur when coming out of a suspend state.

[flackr, 2014/07/29 16:41:45]

This seems problematic, so we can get a lid closed event, suspend, and then wake
up without getting a lid open event? If so, please file (and reference) a bug
for this.

[bruthig, 2014/07/31 16:10:49]

This shouldn't happen, removed.

+  lid_is_closed_ = false;

[flackr, 2014/07/29 16:41:45]

If lid_is_closed_ was true we probably need to set last_lid_open_time_ to be
now, right? Assuming we don't get the open event.

+}
+#endif  // OS_CHROMEOS
+
 void MaximizeModeController::HandleHingeRotation(const gfx::Vector3dF& base,
                                                  const gfx::Vector3dF& lid) {
   static const gfx::Vector3dF hinge_vector(0.0f, 1.0f, 0.0f);
   bool maximize_mode_engaged = IsMaximizeModeWindowManagerEnabled();
   // Ignore the component of acceleration parallel to the hinge for the purposes
   // of hinge angle calculation.
   gfx::Vector3dF base_flattened(base);
   gfx::Vector3dF lid_flattened(lid);
   base_flattened.set_y(0.0f);
   lid_flattened.set_y(0.0f);
 
   // As the hinge approaches a vertical angle, the base and lid accelerometers
   // approach the same values making any angle calculations highly inaccurate.
   // Bail out early when it is too close.
   if (base_flattened.Length() < kHingeAngleDetectionThreshold ||
       lid_flattened.Length() < kHingeAngleDetectionThreshold) {
     return;
   }
 
   // Compute the angle between the base and the lid.
   float angle = ClockwiseAngleBetweenVectorsInDegrees(base_flattened,
       lid_flattened, hinge_vector);
 
+  // Clear the last_lid_open_time_ for a stable reading so that there is less
+  // chance of a delay if the lid is moved from the close state to the fully
+  // open state very quickly.
+  if (IsAngleStable(angle))
+    last_lid_open_time_ = base::TimeTicks();
+
   // Toggle maximize mode on or off when corresponding thresholds are passed.
   // TODO(flackr): Make MaximizeModeController own the MaximizeModeWindowManager
   // such that observations of state changes occur after the change and shell
   // has fewer states to track.
-  if (maximize_mode_engaged &&
-      angle > kFullyOpenAngleErrorTolerance &&
-      angle < kExitMaximizeModeAngle) {
+  if (lid_is_closed_ ||

[flackr, 2014/07/29 16:41:44]

LeaveMaximizeMode is called from LidEventReceived, why do we need to call it
again here?

[bruthig, 2014/07/31 16:10:48]

Done.

+      (maximize_mode_engaged &&
+        ((IsAngleStable(angle) && angle < kExitMaximizeModeAngle) ||
+          (angle > kMaxStableAngle && WasLidOpenedRecently())))) {

[flackr, 2014/07/29 16:41:44]

If we call LeaveMaximizeMode on lid opened (which seems to be the case), and we
won't re-enter maximize mode until we get a stable angle, do we need this last
line of the condition?

[bruthig, 2014/07/31 16:10:49]

Done.

     LeaveMaximizeMode();
   } else if (!maximize_mode_engaged &&

[flackr, 2014/07/29 16:41:44]

Probably could add !lid_is_closed_ here though.

[bruthig, 2014/07/31 16:10:49]

Done.

-      angle > kEnterMaximizeModeAngle) {
+             ((IsAngleStable(angle) && angle > kEnterMaximizeModeAngle) ||
+               (angle > kMaxStableAngle && !WasLidOpenedRecently()))) {
     EnterMaximizeMode();
   }
 }
 
 void MaximizeModeController::HandleScreenRotation(const gfx::Vector3dF& lid) {
   bool maximize_mode_engaged = IsMaximizeModeWindowManagerEnabled();
 
   // TODO(jonross): track the updated rotation angle even when locked. So that
   // when rotation lock is removed the accelerometer rotation can be applied
   // without waiting for the next update.
@@ skipping 58 matching lines @@
     DisplayManager* display_manager,
     gfx::Display::Rotation rotation) {
   base::AutoReset<bool> auto_in_set_screen_rotation(
       &in_set_screen_rotation_, true);
   current_rotation_ = rotation;
   display_manager->SetDisplayRotation(gfx::Display::InternalDisplayId(),
                                       rotation);
 }
 
 void MaximizeModeController::EnterMaximizeMode() {
+  if (IsMaximizeModeWindowManagerEnabled())
+    return;
   DisplayManager* display_manager = Shell::GetInstance()->display_manager();
   current_rotation_ = user_rotation_ = display_manager->
       GetDisplayInfo(gfx::Display::InternalDisplayId()).rotation();
   EnableMaximizeModeWindowManager(true);
 #if defined(USE_X11)
   event_blocker_.reset(new ScopedDisableInternalMouseAndKeyboardX11);
 #endif
 #if defined(OS_CHROMEOS)
   event_handler_.reset(new ScreenshotActionHandler);
 #endif
   Shell::GetInstance()->display_controller()->AddObserver(this);
 }
 
 void MaximizeModeController::LeaveMaximizeMode() {
+  if (!IsMaximizeModeWindowManagerEnabled())
+    return;
   DisplayManager* display_manager = Shell::GetInstance()->display_manager();
   gfx::Display::Rotation current_rotation = display_manager->
       GetDisplayInfo(gfx::Display::InternalDisplayId()).rotation();
   if (current_rotation != user_rotation_)
     SetDisplayRotation(display_manager, user_rotation_);
   rotation_locked_ = false;
   EnableMaximizeModeWindowManager(false);
   event_blocker_.reset();
   event_handler_.reset();
-}
-
-void MaximizeModeController::OnSuspend() {
-  RecordTouchViewStateTransition();

[flackr, 2014/07/29 16:41:45]

Was this never called before? There are no removed observers.

[bruthig, 2014/07/31 16:10:49]

No it wasn't being called.  Appears to have changed with www.crbug.com/359619.

-}
-
-void MaximizeModeController::OnResume() {
-  last_touchview_transition_time_ = base::Time::Now();
+  Shell::GetInstance()->display_controller()->RemoveObserver(this);
 }
 
 // Called after maximize mode has started, windows might still animate though.
 void MaximizeModeController::OnMaximizeModeStarted() {
   RecordTouchViewStateTransition();
 }
 
 // Called after maximize mode has ended, windows might still be returning to
 // their original position.
 void MaximizeModeController::OnMaximizeModeEnded() {
@@ skipping 27 matching lines @@
     base::TimeDelta total_runtime = total_touchview_time_ +
         total_non_touchview_time_;
     if (total_runtime.InSeconds() > 0) {
       UMA_HISTOGRAM_PERCENTAGE("Ash.TouchView.TouchViewActivePercentage",
           100 * total_touchview_time_.InSeconds() / total_runtime.InSeconds());
     }
   }
   Shell::GetInstance()->display_controller()->RemoveObserver(this);
 }
 
+bool MaximizeModeController::WasLidOpenedRecently() const {
+  if (last_lid_open_time_.is_null())
+    return false;
+
+  base::TimeTicks now = tick_clock_->NowTicks();
+  DCHECK(now >= last_lid_open_time_);
+  base::TimeDelta elapsed_time = now - last_lid_open_time_;
+  return elapsed_time <= kLidRecentlyOpenedTolerance;
+}
+
+void MaximizeModeController::SetTickClockForTest(base::TickClock* tick_clock) {
+  DCHECK(tick_clock_);
+  tick_clock_.reset(tick_clock);
+}
+
 }  // namespace ash