Issue 658390 link

Starred by 1 user

Issue metadata

Status:	Archived
Owner:	█ warx@chromium.org Last visit > 30 days ago
Closed:	Oct 2016
Cc:	derat@chromium.org █ bartfab@chromium.org
EstimatedDays:	----
NextAction:	----
OS:	Chrome
Pri:	1
Type:	Bug
VerifyIn-57 VerifyIn-58 VerifyIn-59 VerifyIn-60 VerifyIn-61

Rename ShouldLockScreenBeforeSuspending?

Project Member Reported by warx@chromium.org, Oct 21 2016

Issue description

In SessionStateDelegate, consider renaming ShouldLockScreenBeforeSuspending to ShouldLockScreenIfRequired or ShouldLockScreenIfRequiredInPref.

For two reasons:
(1) In the implementation, https://cs.chromium.org/chromium/src/chrome/browser/ui/ash/session_state_delegate_chromeos.cc?dr=CSs&sq=package:chromium&rcl=1477021054&l=106, I don't see the function body is related to "before suspending". It should also be used in other cases, just like (2).

(2) In tablet power button behavior feature ( crbug.com/633304 ), a power button tap should check ShouldLockScreenIfRequired (Require password to wake from sleep), and then lock the screen if it is required.

What do you think? The other solution might be exposing another api for tablet power button tap. And both of them use the same helper method. But I don't think it is necessary.

Comment 1 by derat@chromium.org, Oct 21 2016

Cc: bartfab@chromium.org
Labels: OS-Chrome

How about ShouldLockScreenAutomatically? It checks prefs::kEnableAutoScreenLock.

I think that this pref corresponds to the "Require password to wake from sleep" setting in the UI, so from the user's perspective, it's just about suspend. It was initially just used for that, I think, but now it also controls whether we lock the screen 10 seconds after the display is turned off due to inactivity (which can happen long before the system goes to sleep).

I agree that we should use this same pref for lock-on-tablet-power-button-tap, so renaming to something more generic like ShouldLockScreenAutomatically makes sense to me.

Comment 2 by warx@chromium.org, Oct 21 2016

ShouldLockScreenAutomatically sounds good to me.

Project Member

Comment 3 by bugdroid1@chromium.org, Oct 25 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b3fd59ca9f44740cee6110dfde3ebd2c2f792ff5

commit b3fd59ca9f44740cee6110dfde3ebd2c2f792ff5
Author: warx <warx@chromium.org>
Date: Tue Oct 25 04:06:07 2016

Refactor ShouldLockScreenBeforeSuspending to ShouldLockScreenAutomatically

CL reasoning:
(1) in line with the pref setting name prefs::kEnableAutoScreenLock
(2) making it more generic for other call usages

BUG= 658390 
TEST=trybot test

Review-Url: https://codereview.chromium.org/2446453002
Cr-Commit-Position: refs/heads/master@{#427273}

[modify] https://crrev.com/b3fd59ca9f44740cee6110dfde3ebd2c2f792ff5/ash/common/session/session_state_delegate.h
[modify] https://crrev.com/b3fd59ca9f44740cee6110dfde3ebd2c2f792ff5/ash/common/test/test_session_state_delegate.cc
[modify] https://crrev.com/b3fd59ca9f44740cee6110dfde3ebd2c2f792ff5/ash/common/test/test_session_state_delegate.h
[modify] https://crrev.com/b3fd59ca9f44740cee6110dfde3ebd2c2f792ff5/ash/mus/bridge/wm_shell_mus.cc
[modify] https://crrev.com/b3fd59ca9f44740cee6110dfde3ebd2c2f792ff5/ash/mus/shell_delegate_mus.cc
[modify] https://crrev.com/b3fd59ca9f44740cee6110dfde3ebd2c2f792ff5/ash/shell/shell_delegate_impl.cc
[modify] https://crrev.com/b3fd59ca9f44740cee6110dfde3ebd2c2f792ff5/ash/system/chromeos/power/power_event_observer.cc
[modify] https://crrev.com/b3fd59ca9f44740cee6110dfde3ebd2c2f792ff5/ash/system/chromeos/power/power_event_observer_unittest.cc
[modify] https://crrev.com/b3fd59ca9f44740cee6110dfde3ebd2c2f792ff5/ash/test/ash_test_base.cc
[modify] https://crrev.com/b3fd59ca9f44740cee6110dfde3ebd2c2f792ff5/ash/test/ash_test_base.h
[modify] https://crrev.com/b3fd59ca9f44740cee6110dfde3ebd2c2f792ff5/chrome/browser/ui/ash/session_state_delegate_chromeos.cc
[modify] https://crrev.com/b3fd59ca9f44740cee6110dfde3ebd2c2f792ff5/chrome/browser/ui/ash/session_state_delegate_chromeos.h
[modify] https://crrev.com/b3fd59ca9f44740cee6110dfde3ebd2c2f792ff5/chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc