ash: Drop the immediately following power button released induced forcing off request

ash/system/chromeos/power/tablet_power_button_controller.cc

 // Copyright 2016 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/system/chromeos/power/tablet_power_button_controller.h"
 
 #include "ash/common/accessibility_delegate.h"
 #include "ash/common/session/session_state_delegate.h"
 #include "ash/common/shell_delegate.h"
 #include "ash/common/wm/maximize_mode/maximize_mode_controller.h"
@@ skipping 17 matching lines @@
 // TODO(derat): This is currently set to a high value to work around delays in
 // powerd's reports of button-up events when the preceding button-down event
 // turns the display on. Set it to a lower value once powerd no longer blocks on
 // asking Chrome to turn the display on: http://crbug.com/685734
 constexpr int kShutdownWhenScreenOffTimeoutMs = 2000;
 
 // Amount of time since last SuspendDone() that power button event needs to be
 // ignored.
 constexpr int kIgnorePowerButtonAfterResumeMs = 2000;
 
+// Amount of time since last power button released that power button released
+// event for forcing off needs to be ignored.
+constexpr int kIgnoreReleasedAfterLastOneMs = 500;

[Daniel Erat, 2017/01/30 17:55:00]

nit: kIgnoreRepeatedButtonUpMs or something similar?

i'd also change the comment to something like:

// Ignore button-up events occurring within this many milliseconds
// of the previous button-up event. This prevents us from falling
// behind if the power button is pressed repeatedly.

[Qiang(Joe) Xu, 2017/01/30 19:35:43]

done, thanks!

+
 // Returns true if device is a convertible/tablet device, otherwise false.
 bool IsTabletModeSupported() {
   MaximizeModeController* maximize_mode_controller =
       WmShell::Get()->maximize_mode_controller();
   return maximize_mode_controller &&
          maximize_mode_controller->CanEnterMaximizeMode();
 }
 
 // Returns true if device is currently in tablet/maximize mode, otherwise false.
 bool IsTabletModeActive() {
@@ skipping 18 matching lines @@
 void TabletPowerButtonController::TestApi::TriggerShutdownTimeout() {
   DCHECK(ShutdownTimerIsRunning());
   controller_->OnShutdownTimeout();
   controller_->shutdown_timer_.Stop();
 }
 
 TabletPowerButtonController::TabletPowerButtonController(
     LockStateController* controller)
     : tick_clock_(new base::DefaultTickClock()),
       last_resume_time_(base::TimeTicks()),
+      last_released_time_(base::TimeTicks()),

[Daniel Erat, 2017/01/30 17:55:00]

i don't think you need to initialize this member and last_resume_time_
explicitly; they should automatically be constructed with the zero value.

[Qiang(Joe) Xu, 2017/01/30 19:35:44]

Done.

       force_off_on_button_up_(true),
       controller_(controller),
       weak_ptr_factory_(this) {
   chromeos::DBusThreadManager::Get()->GetPowerManagerClient()->AddObserver(
       this);
   WmShell::Get()->AddShellObserver(this);
   // TODO(mash): Provide a way for this class to observe stylus events:
   // http://crbug.com/682460
   if (ui::InputDeviceManager::HasInstance())
     ui::InputDeviceManager::GetInstance()->AddObserver(this);
@@ skipping 26 matching lines @@
     // that woke the system. Avoid forcing off display just after resuming to
     // ensure that we don't turn the display off in response to the events.
     if (timestamp - last_resume_time_ <=
         base::TimeDelta::FromMilliseconds(kIgnorePowerButtonAfterResumeMs)) {
       force_off_on_button_up_ = false;
     }
     screen_off_when_power_button_down_ = brightness_level_is_zero_;
     SetDisplayForcedOff(false);
     StartShutdownTimer();
   } else {
+    // When power button is released, cancel shutdown animation whenever it is
+    // still cancellable.
+    if (controller_->CanCancelShutdownAnimation())
+      controller_->CancelShutdownAnimation();
+

[Daniel Erat, 2017/01/30 17:55:00]

might be a little cleaner to do something like this:

  const base::TimeTicks previous_up_time = last_button_up_time_;
  last_button_up_time_ = tick_clock_->NowTicks();

  if (timestamp - previous_up_time <= ...) {
    shutdown_timer_.Stop();
    return;
  }

then the member is only updated by a single statement, which feels a bit
simpler.

[Qiang(Joe) Xu, 2017/01/30 19:35:43]

Done.

+    // Drop the very adjacent power button released event for forcing off.

[Daniel Erat, 2017/01/30 17:55:00]

maybe:

  // Ignore the event if it comes too soon after the last one.

?

[Qiang(Joe) Xu, 2017/01/30 19:35:43]

Done.

+    if (timestamp - last_released_time_ <=
+        base::TimeDelta::FromMilliseconds(kIgnoreReleasedAfterLastOneMs)) {
+      last_released_time_ = tick_clock_->NowTicks();
+      shutdown_timer_.Stop();
+      return;
+    }
+    last_released_time_ = tick_clock_->NowTicks();
+
     if (shutdown_timer_.IsRunning()) {
       shutdown_timer_.Stop();
       if (!screen_off_when_power_button_down_ && force_off_on_button_up_) {
         SetDisplayForcedOff(true);
         LockScreenIfRequired();
       }
     }
-
-    // When power button is released, cancel shutdown animation whenever it is
-    // still cancellable.
-    if (controller_->CanCancelShutdownAnimation())
-      controller_->CancelShutdownAnimation();
   }
 }
 
 void TabletPowerButtonController::PowerManagerRestarted() {
   chromeos::DBusThreadManager::Get()
       ->GetPowerManagerClient()
       ->SetBacklightsForcedOff(backlights_forced_off_);
 }
 
 void TabletPowerButtonController::BrightnessChanged(int level,
@@ skipping 99 matching lines @@
       WmShell::Get()->GetSessionStateDelegate();
   if (session_state_delegate->ShouldLockScreenAutomatically() &&
       session_state_delegate->CanLockScreen() &&
       !session_state_delegate->IsUserSessionBlocked() &&
       !controller_->LockRequested()) {
     session_state_delegate->LockScreen();
   }
 }
 
 }  // namespace ash