ash: Start shutdown timer on post-resume power button press

ash/system/chromeos/power/tablet_power_button_controller.cc

Index: ash/system/chromeos/power/tablet_power_button_controller.cc
diff --git a/ash/system/chromeos/power/tablet_power_button_controller.cc b/ash/system/chromeos/power/tablet_power_button_controller.cc
index 66841d16597b65f645e36c4b68846202ecc4625e..00b1d5f9dd4f661f686b2f873aa96fc921edfdc4 100644
--- a/ash/system/chromeos/power/tablet_power_button_controller.cc
+++ b/ash/system/chromeos/power/tablet_power_button_controller.cc
@@ -25,9 +25,9 @@ namespace {
 // animation when in tablet mode.
 constexpr int kShutdownTimeoutMs = 1000;
 
-// Amount of time since last SuspendDone() that power button event needs to be
-// ignored.
-constexpr int kIgnorePowerButtonAfterResumeMs = 2000;
+// Amount of time that power button pressed is considered as the wake source
+// for suspend.
+constexpr int kDurationPowerButtonAsWakeSourceMs = 2000;

[Daniel Erat, 2016/12/05 17:25:59]

this name and comment don't make that much sense to me now. "wake source" has a
specific meaning in the kernel and this doesn't seem to align with it.

the old name and comment made more sense to me. why do they need to be changed?

[Qiang(Joe) Xu, 2016/12/05 19:33:58]

Because I think the code now is not ignoring the entire OnPowerButtonEvent, it
needs special processing for resuming case. However, "ignore power button event"
seems fine to me because we ignore the forcing off on power button up and we
have documents on the code. So I restore the naming here.

 
 // Returns true if device is a convertible/tablet device or has
 // kAshEnableTouchViewTesting in test, otherwise false.
@@ -46,6 +46,11 @@ bool IsTabletModeActive() {
          maximize_mode_controller->IsMaximizeModeWindowManagerEnabled();
 }
 
+void SendA11yScreenAlert(bool alert_screen_off) {
+  WmShell::Get()->accessibility_delegate()->TriggerAccessibilityAlert(
+      alert_screen_off ? A11Y_ALERT_SCREEN_OFF : A11Y_ALERT_SCREEN_ON);

[Daniel Erat, 2016/12/05 17:26:00]

is there any way to test that alerts are sent at the correct time (e.g. a stub
delegate that logs the alerts it would emit)?

[Qiang(Joe) Xu, 2016/12/05 19:33:58]

mm.. I don't find it. From the CL I found, there is also no test there.
https://codereview.chromium.org/2488373002/.

+}
+
 }  // namespace
 
 TabletPowerButtonController::TestApi::TestApi(
@@ -68,6 +73,7 @@ TabletPowerButtonController::TabletPowerButtonController(
     LockStateController* controller)
     : tick_clock_(new base::DefaultTickClock()),
       last_resume_time_(base::TimeTicks()),
+      ignore_forcing_off_(false),
       controller_(controller),
       weak_ptr_factory_(this) {
   chromeos::DBusThreadManager::Get()->GetPowerManagerClient()->AddObserver(
@@ -92,35 +98,33 @@ bool TabletPowerButtonController::ShouldHandlePowerButtonEvents() const {
 void TabletPowerButtonController::OnPowerButtonEvent(
     bool down,
     const base::TimeTicks& timestamp) {
-  // When the system resumes in response to the power button being pressed,
-  // Chrome receives powerd's SuspendDone signal and notification that the
-  // backlight has been turned back on before seeing the power button events
-  // that woke the system. Ignore events just after resuming to ensure that we
-  // don't turn the screen off in response to the events.
-  //
-  // TODO(warx): pressing power button should also StartShutdownTimer() in this
-  // case. Reorganize the code to support that.
-  if (timestamp - last_resume_time_ <=
-      base::TimeDelta::FromMilliseconds(kIgnorePowerButtonAfterResumeMs)) {
-    // If backlights are forced off, stop forcing off because resuming system
-    // doesn't handle this.
-    if (down && backlights_forced_off_)
-      SetDisplayForcedOff(false);
-    return;
-  }
-
   if (down) {
+    // When the system resumes in response to the power button being pressed,
+    // Chrome receives powerd's SuspendDone signal and notification that the
+    // baclight has been turned back on before seeing the power button events
+    // that woke the system. Ignore forcing off display just after resuming to
+    // ensure that we don't turn the screen off in response to such event.
+    if (timestamp - last_resume_time_ <=
+        base::TimeDelta::FromMilliseconds(kDurationPowerButtonAsWakeSourceMs)) {
+      ignore_forcing_off_ = true;
+      // When |backlights_forced_off_| is false and system resumes in response
+      // to the power button being pressed, the following SetDisplayForcedOff
+      // call will not send a11y alert because of the early return.
+      if (!backlights_forced_off_)
+        SendA11yScreenAlert(false);

[Daniel Erat, 2016/12/05 17:26:00]

why do you need to call this here? won't this make us announce that the screen
is on even though we didn't announce that it was off?

[Qiang(Joe) Xu, 2016/12/05 19:33:58]

If we want "tablet power button event" always trigger the alert if there is a
screen on-off state change, this line is needed.

But you are right, if the a11y alert is bonded to SetBacklightsForcedOff, then
this line is not needed otherwise the alert is not paired.

I start to believe we don't need it now since the a11y alert is introduced with
the rule that it is bound to "forcing off". It is better to keep this rule right
now.

+    }
     screen_off_when_power_button_down_ = brightness_level_is_zero_;
     SetDisplayForcedOff(false);
     StartShutdownTimer();
   } else {
     if (shutdown_timer_.IsRunning()) {
       shutdown_timer_.Stop();
-      if (!screen_off_when_power_button_down_) {
+      if (!screen_off_when_power_button_down_ && !ignore_forcing_off_) {
         SetDisplayForcedOff(true);
         LockScreenIfRequired();
       }
     }
+    ignore_forcing_off_ = false;

[Daniel Erat, 2016/12/05 17:26:00]

how about just setting this to true/false in the down handler? it feels a bit
cleaner to only set it in one place.

[Qiang(Joe) Xu, 2016/12/05 19:33:58]

Done.

     screen_off_when_power_button_down_ = false;
 
     // When power button is released, cancel shutdown animation whenever it is
@@ -196,9 +200,7 @@ void TabletPowerButtonController::SetDisplayForcedOff(bool forced_off) {
                                          true /* use_local_state */);
   delegate->UpdateTouchscreenStatusFromPrefs();
 
-  // Send an a11y alert.
-  WmShell::Get()->accessibility_delegate()->TriggerAccessibilityAlert(
-      forced_off ? A11Y_ALERT_SCREEN_OFF : A11Y_ALERT_SCREEN_ON);
+  SendA11yScreenAlert(forced_off);
 }
 
 void TabletPowerButtonController::GetInitialBacklightsForcedOff() {