ash: Add one second grace period For LockScreenIfRequired

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 4600b46b0538b2281b42cdbe0a24bc234e3cf639..1213ef4512b4080b1edc039dd9dea618dbf3078b 100644
--- a/ash/system/chromeos/power/tablet_power_button_controller.cc
+++ b/ash/system/chromeos/power/tablet_power_button_controller.cc
@@ -25,6 +25,10 @@ namespace {
 // animation when in tablet mode.
 constexpr int kShutdownTimeoutMs = 1000;
 
+// Amount of time that locking screen needs to be waited to start the actual

[Daniel Erat, 2017/01/11 23:09:06]

this comment is a bit confusing. is it more like:

"Amount of time that power button must be held before screen is locked."

?

[Qiang(Joe) Xu, 2017/01/12 00:04:45]

That is not right. This cl is about: "after set display off, and wait 1s, then
perform locking screen action if required; but if there is a
SetDisplayOff(false) call before lock_screen_timer_ times out, stop the timer so
the OnLockScreenTimeout will not get called".

+// locking screen.
+constexpr int kLockScreenTimeoutMs = 1000;
+
 // Amount of time since last SuspendDone() that power button event needs to be
 // ignored.
 constexpr int kIgnorePowerButtonAfterResumeMs = 2000;
@@ -64,6 +68,16 @@ void TabletPowerButtonController::TestApi::TriggerShutdownTimeout() {
   controller_->shutdown_timer_.Stop();
 }
 
+bool TabletPowerButtonController::TestApi::LockScreenTimerIsRunning() const {
+  return controller_->lock_screen_timer_.IsRunning();
+}
+
+void TabletPowerButtonController::TestApi::TriggerLockScreenTimeout() {
+  DCHECK(LockScreenTimerIsRunning());
+  controller_->OnLockScreenTimeout();
+  controller_->lock_screen_timer_.Stop();
+}
+
 TabletPowerButtonController::TabletPowerButtonController(
     LockStateController* controller)
     : tick_clock_(new base::DefaultTickClock()),
@@ -206,6 +220,11 @@ void TabletPowerButtonController::SetDisplayForcedOff(bool forced_off) {
   // Send an a11y alert.
   WmShell::Get()->accessibility_delegate()->TriggerAccessibilityAlert(
       forced_off ? A11Y_ALERT_SCREEN_OFF : A11Y_ALERT_SCREEN_ON);
+
+  // Reset |lock_screen_timer_| when display is not set to forced off since

[Daniel Erat, 2017/01/11 23:09:06]

this doesn't feel like the right place for this code. the method is named
SetDisplayForcedOff, so stopping the screen-lock timer feels like an unexpected
side effect. i'd recommend instead calling lock_screen_timer_.Stop() directly in
all the places where you call SetDisplayForcedOff(false).

with that said, i'm not sure that it makes sense to stop the timer if a key is
pressed or the mouse is used while the power button is held. maybe i'm
misunderstanding this, though. what's the use case that you're trying to handle
here?

[Qiang(Joe) Xu, 2017/01/12 00:04:45]

Agree that we should put it in all the places where we call
SetDisplayForcedOff(false).

As explained in the above, so the quick "two power button tapping /  power
button tapping + keyboard event / power button tapping + mouse event" will not
turn out as locked screen with display on.

+  // under this case, |lock_screen_timer_| should not timeout.

[Daniel Erat, 2017/01/11 23:09:06]

nit: s/timeout/time out/ ("time out" is verb, "timeout" is noun)

[Qiang(Joe) Xu, 2017/01/12 00:04:45]

done by removing.

+  if (!forced_off)
+    lock_screen_timer_.Stop();

[Qiang(Joe) Xu, 2017/01/11 22:17:06]

Moved here: since besides two quick power button tapping, power button tap +
keyboard event, power button + mouse event should also not lock screen.

 }
 
 void TabletPowerButtonController::GetInitialBacklightsForcedOff() {
@@ -238,8 +257,18 @@ void TabletPowerButtonController::LockScreenIfRequired() {
       session_state_delegate->CanLockScreen() &&
       !session_state_delegate->IsUserSessionBlocked() &&
       !controller_->LockRequested()) {
-    session_state_delegate->LockScreen();
+    StartLockScreenTimer();
   }
 }
 
+void TabletPowerButtonController::StartLockScreenTimer() {
+  lock_screen_timer_.Start(

[Daniel Erat, 2017/01/11 23:09:06]

mind moving this into LockScreenIfRequired since it's a single statement and
looks like it's only called from a single place?

[Qiang(Joe) Xu, 2017/01/12 00:04:45]

Done.

+      FROM_HERE, base::TimeDelta::FromMilliseconds(kLockScreenTimeoutMs), this,
+      &TabletPowerButtonController::OnLockScreenTimeout);
+}
+
+void TabletPowerButtonController::OnLockScreenTimeout() {
+  WmShell::Get()->GetSessionStateDelegate()->LockScreen();
+}
+
 }  // namespace ash