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

ash/wm/maximize_mode/maximize_mode_controller.cc

Index: ash/wm/maximize_mode/maximize_mode_controller.cc
diff --git a/ash/wm/maximize_mode/maximize_mode_controller.cc b/ash/wm/maximize_mode/maximize_mode_controller.cc
index 36f6f2514ca137a01e7595f34a8e5176994367ae..3801bd59d5d8222b315ace1317c8bb3adb40775b 100644
--- a/ash/wm/maximize_mode/maximize_mode_controller.cc
+++ b/ash/wm/maximize_mode/maximize_mode_controller.cc
@@ -15,6 +15,7 @@
 #include "base/auto_reset.h"
 #include "base/command_line.h"
 #include "base/metrics/histogram.h"
+#include "chromeos/dbus/dbus_thread_manager.h"

[jonross, 2014/07/24 14:22:00]

Can you confirm via gyp files that this will always compile in when
MaximizeModeController does?

If not we will need to separate this out.

[bruthig, 2014/07/24 19:57:06]

Done.

 #include "ui/base/accelerators/accelerator.h"
 #include "ui/events/event.h"
 #include "ui/events/event_handler.h"
@@ -36,11 +37,18 @@ const float kEnterMaximizeModeAngle = 200.0f;
 // 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 in seconds 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 int kLidRecentlyOpenedTolerance = 2;

[jonross, 2014/07/24 14:22:00]

For constants representing a time duration we append a representation for the
time "Ms" for milliseconds, "Seconds" for seconds.

Could you update this constant?

[bruthig, 2014/07/24 19:57:06]

Done.

 
 // When the device approaches vertical orientation (i.e. portrait orientation)
 // the accelerometers for the base and lid approach the same values (i.e.
@@ -139,19 +147,30 @@ void ScreenshotActionHandler::OnKeyEvent(ui::KeyEvent* event) {
 
 }  // namespace
 
+base::TimeTicks MaximizeModeController::TimeTickProviderImpl::Now() const {
+  return base::TimeTicks::Now();
+}
+
 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_(),
+      time_tick_provider_(new TimeTickProviderImpl()),
+      lid_is_closed_(false) {
   Shell::GetInstance()->accelerometer_controller()->AddObserver(this);
   Shell::GetInstance()->AddShellObserver(this);
+  chromeos::DBusThreadManager::Get()->
+    GetPowerManagerClient()->AddObserver(this);
 }
 
 MaximizeModeController::~MaximizeModeController() {
   Shell::GetInstance()->RemoveShellObserver(this);
   Shell::GetInstance()->accelerometer_controller()->RemoveObserver(this);
+  chromeos::DBusThreadManager::Get()->
+    GetPowerManagerClient()->RemoveObserver(this);
 }
 
 void MaximizeModeController::SetRotationLocked(bool rotation_locked) {
@@ -239,10 +258,28 @@ void MaximizeModeController::OnDisplayConfigurationChanged() {
   }
 }
 
+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.

[jonross, 2014/07/24 14:22:00]

If it does not, is there a way to poll lid state upon resuming from suspend?

[bruthig, 2014/07/24 19:57:06]

I'm not sure if there is a way or not but I don't think it is necessary.  As far
as I know you can't come out of suspend when the lid is closed and thus such a
poll would always return "not open" and would be useless.  Explicitly setting
"lid_is_closed_ = false" here is handling the case when we are already in a
maximize mode position and the user hits the power button.

[jonross, 2014/07/25 13:39:06]

Acknowledged.

+  lid_is_closed_ = false;
+}
+
 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);
@@ -261,18 +298,29 @@ void MaximizeModeController::HandleHingeRotation(const gfx::Vector3dF& base,
   // Compute the angle between the base and the lid.
   float angle = ClockwiseAngleBetweenVectorsInDegrees(base_flattened,
       lid_flattened, hinge_vector);
-
   // 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_) {
     LeaveMaximizeMode();
-  } else if (!maximize_mode_engaged &&
-      angle > kEnterMaximizeModeAngle) {
-    EnterMaximizeMode();
+  } else if (angle > kMinStableAngle &&

[jonross, 2014/07/24 14:22:00]

Should we exit MaximizeMode if angle <kMinStableAngle?

[bruthig, 2014/07/24 19:57:06]

No we don't want to exit MaximizeMode in this case because when the lid is fully
open we get erroneous readings like 5 degrees and we don't want to flicker in
and out of MaximizeMode when the lid is fully open.

[jonross, 2014/07/25 13:39:06]

Acknowledged.

+             angle < kMaxStableAngle) {
+    // 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.
+    last_lid_open_time_ = base::TimeTicks();

[jonross, 2014/07/24 14:22:00]

This moves the lid open time to be later than when it was received. Would this
not delay state change further?

[bruthig, 2014/07/24 19:57:06]

I don't think this is doing what you think.  It's setting the
last_lid_open_time_ to be "NULL", not Now().

[jonross, 2014/07/25 13:39:06]

My misreading.

+    if (angle < kExitMaximizeModeAngle) {
+      LeaveMaximizeMode();
+    } else if (angle > kEnterMaximizeModeAngle) {
+      EnterMaximizeMode();
+    }
+  } else if (angle > kMaxStableAngle) {
+    if (WasLidOpenedRecently()) {
+      LeaveMaximizeMode();
+    } else {
+      EnterMaximizeMode();
+    }
   }
 }
 
@@ -350,6 +398,8 @@ void MaximizeModeController::SetDisplayRotation(
 }
 
 void MaximizeModeController::EnterMaximizeMode() {
+  if (IsMaximizeModeWindowManagerEnabled())

[jonross, 2014/07/24 14:22:00]

We should stop calling this twice.

[bruthig, 2014/07/24 19:57:06]

In order to stop calling it twice each spot that calls it would have to include
the "if (IsMaximizeModeWindowManagerEnabled()) " guard and this was starting to
bloat the code.

I could create a "TryEnterMaximizeMode()" method that would have the guard and
then call this method, but I really don't like the idea of the code bloat with
guarding every call.

+    return;
   DisplayManager* display_manager = Shell::GetInstance()->display_manager();
   current_rotation_ = user_rotation_ = display_manager->
       GetDisplayInfo(gfx::Display::InternalDisplayId()).rotation();
@@ -364,6 +414,8 @@ void MaximizeModeController::EnterMaximizeMode() {
 }
 
 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();
@@ -373,14 +425,7 @@ void MaximizeModeController::LeaveMaximizeMode() {
   EnableMaximizeModeWindowManager(false);
   event_blocker_.reset();
   event_handler_.reset();
-}
-
-void MaximizeModeController::OnSuspend() {
-  RecordTouchViewStateTransition();
-}
-
-void MaximizeModeController::OnResume() {
-  last_touchview_transition_time_ = base::Time::Now();
+  Shell::GetInstance()->display_controller()->RemoveObserver(this);

[jonross, 2014/07/24 14:22:00]

Nice fix.

 }
 
 // Called after maximize mode has started, windows might still animate though.
@@ -428,4 +473,22 @@ void MaximizeModeController::OnAppTerminating() {
   Shell::GetInstance()->display_controller()->RemoveObserver(this);
 }
 
+bool MaximizeModeController::WasLidOpenedRecently() const {
+  bool was_lid_recently_opened = false;
+  if (!last_lid_open_time_.is_null()) {

[jonross, 2014/07/24 14:22:00]

Quick exits are prefered.

if (last_lid_open_time_.is_null())
  return false;

//Logic below

[bruthig, 2014/07/24 19:57:06]

Done.

+    base::TimeTicks now = time_tick_provider_->Now();
+    DCHECK(now >= last_lid_open_time_);
+    base::TimeDelta elapsed_time = now - last_lid_open_time_;
+    was_lid_recently_opened =
+      elapsed_time <= base::TimeDelta::FromSeconds(kLidRecentlyOpenedTolerance);
+  }
+  return was_lid_recently_opened;
+}
+
+void MaximizeModeController::SetTimeTickProviderForTest(
+    TimeTickProvider* provider) {
+  DCHECK(provider);
+  time_tick_provider_.reset(provider);
+}
+
 }  // namespace ash