Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
15734010:
chromeos: Add delay between screen off and lock. (Closed)
|
Created:
7 years, 7 months ago by Daniel Erat Modified:
7 years, 6 months ago CC:
chromium-reviews, derat+watch_chromium.org, oshima+watch_chromium.org, stevenjb+watch_chromium.org, nkostylev+watch_chromium.org, davemoore+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Descriptionchromeos: Add delay between screen off and lock.
This adds a ten-second delay between the time at which the
screen is turned off due to user inactivity and the time at
which it's locked.
It also fixes an issue where screen wake locks (as used by
video playback or by the chrome.power extension API) wouldn't
clear lock delays.
BUG=239486, 246008
R=bartfab@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=203774
Patch Set 1 #
Total comments: 9
Patch Set 2 : switch to simpler approach using a constant #
Total comments: 4
Patch Set 3 : make screen wake locks unset screen lock delays #
Total comments: 2
Patch Set 4 : add comment describing ms delay in sec #
Messages
Total messages: 13 (0 generated)
https://codereview.chromium.org/15734010/diff/1/chrome/browser/chromeos/polic... File chrome/browser/chromeos/policy/power_policy_browsertest.cc (right): https://codereview.chromium.org/15734010/diff/1/chrome/browser/chromeos/polic... chrome/browser/chromeos/policy/power_policy_browsertest.cc:75: if (user_policy_name == key::kScreenLockDelayAC || This feels gross; let me know if you can think of a better approach.
Discussed this with Bartosz. We think we found a solution that let's us: - keep the current definitions of the prefs and policies - allow for the 10 second delay by default if you only set the enable-screen-lock flag - doesn't make the delay settings dependent on enable-screen-lock The proposal is to: - If the screen lock delays are set, they get honored regardless of enable-screen-lock - If enable-screen-lock is set, but the delays are not, we synthesize a screen lock delay when constructing the power policy. I've put review comments to illustrate the changes. https://codereview.chromium.org/15734010/diff/1/chrome/browser/chromeos/prefe... File chrome/browser/chromeos/preferences.cc (right): https://codereview.chromium.org/15734010/diff/1/chrome/browser/chromeos/prefe... chrome/browser/chromeos/preferences.cc:325: 490000, This would be 0 again. https://codereview.chromium.org/15734010/diff/1/chrome/browser/chromeos/prefe... chrome/browser/chromeos/preferences.cc:345: 370000, Same here. https://codereview.chromium.org/15734010/diff/1/chromeos/dbus/power_policy_co... File chromeos/dbus/power_policy_controller.cc (left): https://codereview.chromium.org/15734010/diff/1/chromeos/dbus/power_policy_co... chromeos/dbus/power_policy_controller.cc:151: // it's turned off due to user inactivity. I guess we want to retain this block. https://codereview.chromium.org/15734010/diff/1/chromeos/dbus/power_policy_co... File chromeos/dbus/power_policy_controller.cc (right): https://codereview.chromium.org/15734010/diff/1/chromeos/dbus/power_policy_co... chromeos/dbus/power_policy_controller.cc:147: values.enable_screen_lock ? values.ac_screen_lock_delay_ms : 0); This would become: if (values.ac_screen_lock_delay_ms > 0) { delays->set_screen_lock_ms(values.ac_screen_lock_delay_ms); } else if (values.enable_screen_lock) { delays->set_screen_lock_ms(values.ac_screen_off_delay_ms + 10000); } https://codereview.chromium.org/15734010/diff/1/chromeos/dbus/power_policy_co... chromeos/dbus/power_policy_controller.cc:155: values.enable_screen_lock ? values.battery_screen_lock_delay_ms : 0); same for battery obviously.
https://codereview.chromium.org/15734010/diff/1/chromeos/dbus/power_policy_co... File chromeos/dbus/power_policy_controller.cc (right): https://codereview.chromium.org/15734010/diff/1/chromeos/dbus/power_policy_co... chromeos/dbus/power_policy_controller.cc:147: values.enable_screen_lock ? values.ac_screen_lock_delay_ms : 0); On 2013/05/31 13:22:46, Mattias Nissler wrote: > This would become: > > if (values.ac_screen_lock_delay_ms > 0) { > delays->set_screen_lock_ms(values.ac_screen_lock_delay_ms); > } else if (values.enable_screen_lock) { > delays->set_screen_lock_ms(values.ac_screen_off_delay_ms + 10000); > } Actually, I think we would want to use the minimum of the two values (if both are configured). The idea of the |screen_lock_delay_ms| pref was that the admin could set it to any value desired, without worrying about whether |screen_lock_delay| is enabled, is longer or is shorter than |screen_lock_delay_ms|. If |screen_lock_delay_ms| is set by the admin, the screen should always lock at that point at the latest.
https://codereview.chromium.org/15734010/diff/1/chromeos/dbus/power_policy_co... File chromeos/dbus/power_policy_controller.cc (right): https://codereview.chromium.org/15734010/diff/1/chromeos/dbus/power_policy_co... chromeos/dbus/power_policy_controller.cc:147: values.enable_screen_lock ? values.ac_screen_lock_delay_ms : 0); On 2013/05/31 13:35:59, bartfab wrote: > On 2013/05/31 13:22:46, Mattias Nissler wrote: > > This would become: > > > > if (values.ac_screen_lock_delay_ms > 0) { > > delays->set_screen_lock_ms(values.ac_screen_lock_delay_ms); > > } else if (values.enable_screen_lock) { > > delays->set_screen_lock_ms(values.ac_screen_off_delay_ms + 10000); > > } > > Actually, I think we would want to use the minimum of the two values (if both > are configured). The idea of the |screen_lock_delay_ms| pref was that the admin > could set it to any value desired, without worrying about whether > |screen_lock_delay| is enabled, is longer or is shorter than > |screen_lock_delay_ms|. > > If |screen_lock_delay_ms| is set by the admin, the screen should always lock at > that point at the latest. Let me try that again, hopefully without confusing pref names this time: Actually, I think we would want to use the minimum of the two values (if both are configured). The idea of the |screen_lock_delay_ms| pref was that the admin could set it to any value desired, without worrying about whether |screen_off_delay_ms| is enabled, is longer or is shorter than |screen_lock_delay_ms|. If |screen_lock_delay_ms| is set by the admin, the screen should always lock at that point at the latest.
https://codereview.chromium.org/15734010/diff/1/chromeos/dbus/power_policy_co... File chromeos/dbus/power_policy_controller.cc (right): https://codereview.chromium.org/15734010/diff/1/chromeos/dbus/power_policy_co... chromeos/dbus/power_policy_controller.cc:147: values.enable_screen_lock ? values.ac_screen_lock_delay_ms : 0); On 2013/05/31 13:37:32, bartfab wrote: > On 2013/05/31 13:35:59, bartfab wrote: > > On 2013/05/31 13:22:46, Mattias Nissler wrote: > > > This would become: > > > > > > if (values.ac_screen_lock_delay_ms > 0) { > > > delays->set_screen_lock_ms(values.ac_screen_lock_delay_ms); > > > } else if (values.enable_screen_lock) { > > > delays->set_screen_lock_ms(values.ac_screen_off_delay_ms + 10000); > > > } > > > > Actually, I think we would want to use the minimum of the two values (if both > > are configured). The idea of the |screen_lock_delay_ms| pref was that the > admin > > could set it to any value desired, without worrying about whether > > |screen_lock_delay| is enabled, is longer or is shorter than > > |screen_lock_delay_ms|. > > > > If |screen_lock_delay_ms| is set by the admin, the screen should always lock > at > > that point at the latest. > > Let me try that again, hopefully without confusing pref names this time: > > Actually, I think we would want to use the minimum of the two values (if > both are configured). The idea of the |screen_lock_delay_ms| pref was > that the admin could set it to any value desired, without worrying about > whether |screen_off_delay_ms| is enabled, is longer or is shorter than > |screen_lock_delay_ms|. > > If |screen_lock_delay_ms| is set by the admin, the screen should always > lock at that point at the latest. Yes, I agree. Like Mattias suggested, I'll just retain the existing blocks of code but add a constant to the screen-off delays. See #1 in http://crbug.com/239486#2.
Please take another look. On the bright side, this approach is much simpler and seems safer for merging to 28.
https://chromiumcodereview.appspot.com/15734010/diff/7001/chromeos/dbus/power... File chromeos/dbus/power_policy_controller.cc (right): https://chromiumcodereview.appspot.com/15734010/diff/7001/chromeos/dbus/power... chromeos/dbus/power_policy_controller.cc:61: const int PowerPolicyController::kScreenLockAfterOffDelayMs = 10000; Should we make this a tunable pref? Ad if so, should it be tunable for AC and battery separately? On the one hand, I see little value in yet another tunable. On the other hand, we have made everything else adjustable. This is the only hard-coded constant. And ironically, it has been the offset between screen-off and screen-lock that has caused us the most headaches. Maybe we should make it tunable so that we can tweak it easier in the future or even let admins control it in enterprise cases? https://chromiumcodereview.appspot.com/15734010/diff/7001/chromeos/dbus/power... File chromeos/dbus/power_policy_controller_unittest.cc (right): https://chromiumcodereview.appspot.com/15734010/diff/7001/chromeos/dbus/power... chromeos/dbus/power_policy_controller_unittest.cc:123: // idle action should lock the screen in this case. Is this also true if the idle action is "do nothing"? Or would we want to lock the screen in this case once screen-off + constant is reached?
https://chromiumcodereview.appspot.com/15734010/diff/7001/chromeos/dbus/power... File chromeos/dbus/power_policy_controller.cc (right): https://chromiumcodereview.appspot.com/15734010/diff/7001/chromeos/dbus/power... chromeos/dbus/power_policy_controller.cc:61: const int PowerPolicyController::kScreenLockAfterOffDelayMs = 10000; On 2013/05/31 17:39:07, bartfab wrote: > Should we make this a tunable pref? Ad if so, should it be tunable for AC and > battery separately? > > On the one hand, I see little value in yet another tunable. On the other hand, > we have made everything else adjustable. This is the only hard-coded constant. > And ironically, it has been the offset between screen-off and screen-lock that > has caused us the most headaches. Maybe we should make it tunable so that we can > tweak it easier in the future or even let admins control it in enterprise cases? No, I don't think that this should be a pref. - Admins would have no need to set this unless they wanted the gap between screen-off and screen-lock to be longer than ten seconds. They can already set the lock delays. - A constant is as easy for us to tweak as a pref's default value. - There's no benefit to making this a pref before we have a need to make it tunable -- it's easy to replace a constant with a pref later. https://chromiumcodereview.appspot.com/15734010/diff/7001/chromeos/dbus/power... File chromeos/dbus/power_policy_controller_unittest.cc (right): https://chromiumcodereview.appspot.com/15734010/diff/7001/chromeos/dbus/power... chromeos/dbus/power_policy_controller_unittest.cc:123: // idle action should lock the screen in this case. On 2013/05/31 17:39:07, bartfab wrote: > Is this also true if the idle action is "do nothing"? Or would we want to lock > the screen in this case once screen-off + constant is reached? Yes, this is also true for "do nothing": http://git.chromium.org/gitweb/?p=chromiumos/platform/power_manager.git;a=blo... The power manager expects the other delays to precede the idle delay. I have no plans to change this to accommodate nonsensical configurations (e.g. screen-off at 5m, do-nothing-idle at 5m5s could just as easily be changed to do-nothing-idle at 10m).
Please take another look; I've added a tiny fix for a related issue.
lgtm https://codereview.chromium.org/15734010/diff/13001/chromeos/dbus/power_polic... File chromeos/dbus/power_policy_controller.cc (right): https://codereview.chromium.org/15734010/diff/13001/chromeos/dbus/power_polic... chromeos/dbus/power_policy_controller.cc:61: const int PowerPolicyController::kScreenLockAfterOffDelayMs = 10000; Nit: We usually add a quick comment at the end of each such time-length constant like: // 10 seconds
https://codereview.chromium.org/15734010/diff/13001/chromeos/dbus/power_polic... File chromeos/dbus/power_policy_controller.cc (right): https://codereview.chromium.org/15734010/diff/13001/chromeos/dbus/power_polic... chromeos/dbus/power_policy_controller.cc:61: const int PowerPolicyController::kScreenLockAfterOffDelayMs = 10000; On 2013/06/03 08:28:59, bartfab wrote: > Nit: We usually add a quick comment at the end of each such time-length constant > like: > > // 10 seconds Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/derat@chromium.org/15734010/20001
Message was sent while issue was closed.
Committed patchset #4 manually as r203774 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
