Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2620383003:
ash: Add one second grace period For LockScreenIfRequired (Closed)
|
Created:
3 years, 11 months ago by Qiang(Joe) Xu Modified:
3 years, 11 months ago Reviewers:
Daniel Erat CC:
chromium-reviews, kalyank, sadrul, oshima+watch_chromium.org, derat+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionash: Add one second grace period For LockScreenIfRequired
Changes:
Add one second grace period For LockScreenIfRequired. Basically, an one second OneShotTimer is used to set/reset the behavior.
BUG=633304
BUG=680509
TEST=device test, and modify test coverage
Review-Url: https://codereview.chromium.org/2620383003
Cr-Commit-Position: refs/heads/master@{#443254}
Committed: https://chromium.googlesource.com/chromium/src/+/88b1b7e758f10253dc293c05006c8695fe58079e
Patch Set 1 #Patch Set 2 : reset |lock_screen_timer_| in SetDisplayForcedOff and add test coverage #
Total comments: 13
Patch Set 3 : rebase #Patch Set 4 : based on ps2's comments #
Total comments: 6
Patch Set 5 : based on ps4's comments #Patch Set 6 : rebase #
Messages
Total messages: 21 (11 generated)
Description was changed from ========== ash: Add one second grace period For LockScreenIfRequired BUG=633304 TEST=device test, and modify test coverage ========== to ========== ash: Add one second grace period For LockScreenIfRequired Changes: Add one second grace period For LockScreenIfRequired. Basically, an one second OneShotTimer is used to set/reset the behavior. BUG=633304 TEST=device test, and modify test coverage ==========
warx@chromium.org changed reviewers: + derat@chromium.org
Hi Daniel, ptal, thanks! https://codereview.chromium.org/2620383003/diff/20001/ash/system/chromeos/pow... File ash/system/chromeos/power/tablet_power_button_controller.cc (right): https://codereview.chromium.org/2620383003/diff/20001/ash/system/chromeos/pow... ash/system/chromeos/power/tablet_power_button_controller.cc:227: lock_screen_timer_.Stop(); Moved here: since besides two quick power button tapping, power button tap + keyboard event, power button + mouse event should also not lock screen.
https://codereview.chromium.org/2620383003/diff/20001/ash/system/chromeos/pow... File ash/system/chromeos/power/tablet_power_button_controller.cc (right): https://codereview.chromium.org/2620383003/diff/20001/ash/system/chromeos/pow... ash/system/chromeos/power/tablet_power_button_controller.cc:28: // Amount of time that locking screen needs to be waited to start the actual this comment is a bit confusing. is it more like: "Amount of time that power button must be held before screen is locked." ? https://codereview.chromium.org/2620383003/diff/20001/ash/system/chromeos/pow... ash/system/chromeos/power/tablet_power_button_controller.cc:224: // Reset |lock_screen_timer_| when display is not set to forced off since 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? https://codereview.chromium.org/2620383003/diff/20001/ash/system/chromeos/pow... ash/system/chromeos/power/tablet_power_button_controller.cc:225: // under this case, |lock_screen_timer_| should not timeout. nit: s/timeout/time out/ ("time out" is verb, "timeout" is noun) https://codereview.chromium.org/2620383003/diff/20001/ash/system/chromeos/pow... ash/system/chromeos/power/tablet_power_button_controller.cc:265: lock_screen_timer_.Start( mind moving this into LockScreenIfRequired since it's a single statement and looks like it's only called from a single place? https://codereview.chromium.org/2620383003/diff/20001/ash/system/chromeos/pow... File ash/system/chromeos/power/tablet_power_button_controller.h (right): https://codereview.chromium.org/2620383003/diff/20001/ash/system/chromeos/pow... ash/system/chromeos/power/tablet_power_button_controller.h:140: // released and it should set display off, and stopped when it timeouts. nit: s/timeouts/times out/ https://codereview.chromium.org/2620383003/diff/20001/ash/system/chromeos/pow... ash/system/chromeos/power/tablet_power_button_controller.h:141: // Run OnLockScreenTimeout() to start locking screen. nit: s/Run/Runs/
Patchset #4 (id:60001) has been deleted
Hi Daniel, ptal, thanks! https://codereview.chromium.org/2620383003/diff/20001/ash/system/chromeos/pow... File ash/system/chromeos/power/tablet_power_button_controller.cc (right): https://codereview.chromium.org/2620383003/diff/20001/ash/system/chromeos/pow... ash/system/chromeos/power/tablet_power_button_controller.cc:28: // Amount of time that locking screen needs to be waited to start the actual On 2017/01/11 23:09:06, Daniel Erat wrote: > this comment is a bit confusing. is it more like: > > "Amount of time that power button must be held before screen is locked." > > ? 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". https://codereview.chromium.org/2620383003/diff/20001/ash/system/chromeos/pow... ash/system/chromeos/power/tablet_power_button_controller.cc:224: // Reset |lock_screen_timer_| when display is not set to forced off since On 2017/01/11 23:09:06, Daniel Erat wrote: > 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? 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. https://codereview.chromium.org/2620383003/diff/20001/ash/system/chromeos/pow... ash/system/chromeos/power/tablet_power_button_controller.cc:225: // under this case, |lock_screen_timer_| should not timeout. On 2017/01/11 23:09:06, Daniel Erat wrote: > nit: s/timeout/time out/ ("time out" is verb, "timeout" is noun) done by removing. https://codereview.chromium.org/2620383003/diff/20001/ash/system/chromeos/pow... ash/system/chromeos/power/tablet_power_button_controller.cc:265: lock_screen_timer_.Start( On 2017/01/11 23:09:06, Daniel Erat wrote: > mind moving this into LockScreenIfRequired since it's a single statement and > looks like it's only called from a single place? Done. https://codereview.chromium.org/2620383003/diff/20001/ash/system/chromeos/pow... File ash/system/chromeos/power/tablet_power_button_controller.h (right): https://codereview.chromium.org/2620383003/diff/20001/ash/system/chromeos/pow... ash/system/chromeos/power/tablet_power_button_controller.h:140: // released and it should set display off, and stopped when it timeouts. On 2017/01/11 23:09:06, Daniel Erat wrote: > nit: s/timeouts/times out/ Done. https://codereview.chromium.org/2620383003/diff/20001/ash/system/chromeos/pow... ash/system/chromeos/power/tablet_power_button_controller.h:141: // Run OnLockScreenTimeout() to start locking screen. On 2017/01/11 23:09:06, Daniel Erat wrote: > nit: s/Run/Runs/ Done.
https://codereview.chromium.org/2620383003/diff/80001/ash/system/chromeos/pow... File ash/system/chromeos/power/tablet_power_button_controller.cc (right): https://codereview.chromium.org/2620383003/diff/80001/ash/system/chromeos/pow... ash/system/chromeos/power/tablet_power_button_controller.cc:29: // forced off. thanks, i understand now. maybe: // Amount of time to delay locking screen after display is forced off. ? https://codereview.chromium.org/2620383003/diff/80001/ash/system/chromeos/pow... ash/system/chromeos/power/tablet_power_button_controller.cc:233: lock_screen_timer_.Stop(); did you mean to remove this now? aren't you already stopping it before calling this method? https://codereview.chromium.org/2620383003/diff/80001/ash/system/chromeos/pow... File ash/system/chromeos/power/tablet_power_button_controller.h (right): https://codereview.chromium.org/2620383003/diff/80001/ash/system/chromeos/pow... ash/system/chromeos/power/tablet_power_button_controller.h:138: // out. Resets timer whenever there is a call of SetDisplayForcedOff(false). this is a pretty complicated comment. could you use something simpler like: // Used to provide a grace period between forcing the display off and // locking the screen. Runs OnLockScreenTimeout(). ? someone can/should look at the code if they want to see the specific logic for when it's started. :-)
Patchset #5 (id:100001) has been deleted
Hi Daniel, ptal, thanks! https://codereview.chromium.org/2620383003/diff/80001/ash/system/chromeos/pow... File ash/system/chromeos/power/tablet_power_button_controller.cc (right): https://codereview.chromium.org/2620383003/diff/80001/ash/system/chromeos/pow... ash/system/chromeos/power/tablet_power_button_controller.cc:29: // forced off. On 2017/01/12 01:40:46, Daniel Erat wrote: > thanks, i understand now. maybe: > > // Amount of time to delay locking screen after display is forced off. > > ? Done. https://codereview.chromium.org/2620383003/diff/80001/ash/system/chromeos/pow... ash/system/chromeos/power/tablet_power_button_controller.cc:233: lock_screen_timer_.Stop(); On 2017/01/12 01:40:46, Daniel Erat wrote: > did you mean to remove this now? aren't you already stopping it before calling > this method? oops, yeh, sorry about this. https://codereview.chromium.org/2620383003/diff/80001/ash/system/chromeos/pow... File ash/system/chromeos/power/tablet_power_button_controller.h (right): https://codereview.chromium.org/2620383003/diff/80001/ash/system/chromeos/pow... ash/system/chromeos/power/tablet_power_button_controller.h:138: // out. Resets timer whenever there is a call of SetDisplayForcedOff(false). On 2017/01/12 01:40:47, Daniel Erat wrote: > this is a pretty complicated comment. could you use something simpler like: > > // Used to provide a grace period between forcing the display off and > // locking the screen. Runs OnLockScreenTimeout(). > > ? someone can/should look at the code if they want to see the specific logic for > when it's started. :-) done, thanks
lgtm
Description was changed from ========== ash: Add one second grace period For LockScreenIfRequired Changes: Add one second grace period For LockScreenIfRequired. Basically, an one second OneShotTimer is used to set/reset the behavior. BUG=633304 TEST=device test, and modify test coverage ========== to ========== ash: Add one second grace period For LockScreenIfRequired Changes: Add one second grace period For LockScreenIfRequired. Basically, an one second OneShotTimer is used to set/reset the behavior. BUG=633304 BUG=680509 TEST=device test, and modify test coverage ==========
The CQ bit was checked by warx@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for
ash/system/chromeos/power/tablet_power_button_controller.cc:
While running git apply --index -p1;
error: patch failed:
ash/system/chromeos/power/tablet_power_button_controller.cc:171
error: ash/system/chromeos/power/tablet_power_button_controller.cc: patch does
not apply
Patch: 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
d8b3fc6f54204c082c8fd1ce0bfc718cc44c84ec..fbcc156119b970a608e92bfa057866afa5b8971f
100644
--- a/ash/system/chromeos/power/tablet_power_button_controller.cc
+++ b/ash/system/chromeos/power/tablet_power_button_controller.cc
@@ -25,6 +25,9 @@ namespace {
// animation when in tablet mode.
constexpr int kShutdownTimeoutMs = 500;
+// Amount of time to delay locking screen after display is forced off.
+constexpr int kLockScreenTimeoutMs = 1000;
+
// Amount of time since last SuspendDone() that power button event needs to be
// ignored.
constexpr int kIgnorePowerButtonAfterResumeMs = 2000;
@@ -64,6 +67,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()),
@@ -108,6 +121,7 @@ void TabletPowerButtonController::OnPowerButtonEvent(
}
screen_off_when_power_button_down_ = brightness_level_is_zero_;
SetDisplayForcedOff(false);
+ lock_screen_timer_.Stop();
StartShutdownTimer();
} else {
if (shutdown_timer_.IsRunning()) {
@@ -159,8 +173,10 @@ void TabletPowerButtonController::OnKeyEvent(ui::KeyEvent*
event) {
if (event->key_code() == ui::VKEY_POWER)
return;
- if (!IsTabletModeActive() && backlights_forced_off_)
+ if (!IsTabletModeActive() && backlights_forced_off_) {
SetDisplayForcedOff(false);
+ lock_screen_timer_.Stop();
+ }
}
void TabletPowerButtonController::OnMouseEvent(ui::MouseEvent* event) {
@@ -171,14 +187,17 @@ void
TabletPowerButtonController::OnMouseEvent(ui::MouseEvent* event) {
return;
}
- if (!IsTabletModeActive() && backlights_forced_off_)
+ if (!IsTabletModeActive() && backlights_forced_off_) {
SetDisplayForcedOff(false);
+ lock_screen_timer_.Stop();
+ }
}
void TabletPowerButtonController::OnStylusStateChanged(ui::StylusState state) {
if (IsTabletModeSupported() && state == ui::StylusState::REMOVED &&
backlights_forced_off_) {
SetDisplayForcedOff(false);
+ lock_screen_timer_.Stop();
}
}
@@ -238,8 +257,14 @@ void TabletPowerButtonController::LockScreenIfRequired() {
session_state_delegate->CanLockScreen() &&
!session_state_delegate->IsUserSessionBlocked() &&
!controller_->LockRequested()) {
- session_state_delegate->LockScreen();
+ lock_screen_timer_.Start(
+ FROM_HERE, base::TimeDelta::FromMilliseconds(kLockScreenTimeoutMs),
+ this, &TabletPowerButtonController::OnLockScreenTimeout);
}
}
+void TabletPowerButtonController::OnLockScreenTimeout() {
+ WmShell::Get()->GetSessionStateDelegate()->LockScreen();
+}
+
} // namespace ash
The CQ bit was checked by warx@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from derat@chromium.org Link to the patchset: https://codereview.chromium.org/2620383003/#ps140001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1484236050818050,
"parent_rev": "7a2e011d044d69f03989cea779d8e8a1f4d2af91", "commit_rev":
"88b1b7e758f10253dc293c05006c8695fe58079e"}
Message was sent while issue was closed.
Description was changed from ========== ash: Add one second grace period For LockScreenIfRequired Changes: Add one second grace period For LockScreenIfRequired. Basically, an one second OneShotTimer is used to set/reset the behavior. BUG=633304 BUG=680509 TEST=device test, and modify test coverage ========== to ========== ash: Add one second grace period For LockScreenIfRequired Changes: Add one second grace period For LockScreenIfRequired. Basically, an one second OneShotTimer is used to set/reset the behavior. BUG=633304 BUG=680509 TEST=device test, and modify test coverage Review-Url: https://codereview.chromium.org/2620383003 Cr-Commit-Position: refs/heads/master@{#443254} Committed: https://chromium.googlesource.com/chromium/src/+/88b1b7e758f10253dc293c05006c... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:140001) as https://chromium.googlesource.com/chromium/src/+/88b1b7e758f10253dc293c05006c... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
