New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 758574 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Sep 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Feature



Sign in to add a comment

Turn screen off soon after it's locked via power button

Project Member Reported by derat@chromium.org, Aug 24 2017

Issue description

As discussed in  issue 756601 , we'd like to have a way to use clamshell-style power button behavior on some convertible devices. We can get that now using the --force-clamshell-power-button flag, but it makes it hard to carry around a device while it's in tablet mode: after holding the power button to lock the screen, the screen stays on for 40/80 seconds[1] before turning off. It'd be nice if the screen instead just stayed on for ~2 seconds after the power button is held to lock the screen.

I think that the right way to achieve this is by updating ash::PowerButtonController to start a timer after it locks the screen and later call powerd's SetBacklightsForcedOff D-Bus method. The timer needs to be stopped if user input is observed, and we also need to stop forcing the backlights off in various cases (including user activity). I suspect that much of the logic from ash::TabletPowerButtonController will need to be duplicated here. We may want to pause media sessions as well in this case.

This feels like a somewhat-risky change, so I'll probably look into only changing the behavior when --force-clamshell-power-button is set, at least initially. We might want to apply it across the board later.

1. https://chromium.googlesource.com/chromiumos/platform2/+/master/power_manager/docs/inactivity_delays.md 
 

Comment 1 by warx@chromium.org, Aug 24 2017

Approach sounds good. I am just curious why we want this behavior on some convertible devices. Specifically, what the property the device should have for this feature?

Comment 2 by derat@chromium.org, Aug 25 2017

#1: See  issue 756601  for the specific use case. I'm happy to go into more detail over IM. :-)
Project Member

Comment 3 by bugdroid1@chromium.org, Aug 25 2017

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

commit fb2a6521d2a372b5a87c8b1040caab9ccc4a0c7a
Author: Daniel Erat <derat@chromium.org>
Date: Fri Aug 25 20:01:45 2017

chromeos: Add ash::PowerButtonDisplayController.

Move backlight- and touchscreen-controlling code out of
ash::TabletPowerButtonController and into a new
ash::PowerButtonDisplayController class that can be shared
with ash::PowerButtonController in a later change.

Bug:  758574 
Change-Id: I411067859b822fdee0366b56217255074b42227e
Reviewed-on: https://chromium-review.googlesource.com/634409
Reviewed-by: Qiang(Joe) Xu <warx@chromium.org>
Commit-Queue: Dan Erat <derat@chromium.org>
Cr-Commit-Position: refs/heads/master@{#497510}
[modify] https://crrev.com/fb2a6521d2a372b5a87c8b1040caab9ccc4a0c7a/ash/BUILD.gn
[add] https://crrev.com/fb2a6521d2a372b5a87c8b1040caab9ccc4a0c7a/ash/system/power/power_button_display_controller.cc
[add] https://crrev.com/fb2a6521d2a372b5a87c8b1040caab9ccc4a0c7a/ash/system/power/power_button_display_controller.h
[modify] https://crrev.com/fb2a6521d2a372b5a87c8b1040caab9ccc4a0c7a/ash/system/power/tablet_power_button_controller.cc
[modify] https://crrev.com/fb2a6521d2a372b5a87c8b1040caab9ccc4a0c7a/ash/system/power/tablet_power_button_controller.h
[modify] https://crrev.com/fb2a6521d2a372b5a87c8b1040caab9ccc4a0c7a/ash/system/power/tablet_power_button_controller_unittest.cc

Comment 4 by derat@chromium.org, Sep 19 2017

Status: Started (was: Assigned)

Comment 5 by derat@chromium.org, Sep 19 2017

Sent https://chromium-review.googlesource.com/c/chromium/src/+/671669. I went with 3 seconds rather than 2 since 2 is a bit short -- the timer starts when the power button is released, but locking can take a bit longer.
Project Member

Comment 6 by bugdroid1@chromium.org, Sep 19 2017

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

commit 948c94d13ae3e5dce977c4bdaca54e4712364f93
Author: Daniel Erat <derat@chromium.org>
Date: Tue Sep 19 01:34:30 2017

chromeos: Turn display off after lock via power button.

Make ash::PowerButtonController force the backlights off
three seconds after screen locking starts when the
--force-clamshell-power-button flag is set. This avoids a
long, awkward period where the display is on with the lock
screen visible. The behavior on convertible and clamshell
devices that don't have the flag is unchanged.

Bug:  758574 
Change-Id: I5a645def1205948419d7cbb711fbdbc2daaaf9e7
Reviewed-on: https://chromium-review.googlesource.com/671669
Reviewed-by: Qiang(Joe) Xu <warx@chromium.org>
Commit-Queue: Dan Erat <derat@chromium.org>
Cr-Commit-Position: refs/heads/master@{#502751}
[modify] https://crrev.com/948c94d13ae3e5dce977c4bdaca54e4712364f93/ash/wm/lock_state_controller_unittest.cc
[modify] https://crrev.com/948c94d13ae3e5dce977c4bdaca54e4712364f93/ash/wm/power_button_controller.cc
[modify] https://crrev.com/948c94d13ae3e5dce977c4bdaca54e4712364f93/ash/wm/power_button_controller.h
[modify] https://crrev.com/948c94d13ae3e5dce977c4bdaca54e4712364f93/chromeos/dbus/fake_power_manager_client.h

Comment 7 by derat@chromium.org, Sep 20 2017

Labels: -Pri-2 Merge-Request-62 Pri-1
UX asked for this to be merged to M62.
Project Member

Comment 8 by sheriffbot@chromium.org, Sep 20 2017

Labels: -Merge-Request-62 Hotlist-Merge-Reject Merge-Reject-62
The bug is marked as P3 or Feature. It should not be merged as M62 is in beta. 
Please contact the approriate milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 9 by derat@chromium.org, Sep 20 2017

Cc: bhthompson@chromium.org
I don't think it's the case that M62 is in beta for Chrome OS, which is the only platform affected by this change. Bernie, is that correct?
By the schedule it is well beyond the period we accept features, feature freeze is two weeks before branch which is over a month ago now. 

We have not promoted Chrome OS to beta yet though.

If there is concern this may not be safe, we should probably wait until 63, if we are really confident in this, and we believe it to be necessary enough we would not ship products without it (e.g. we call it a bug), then we can merge it back.

Comment 11 by derat@chromium.org, Sep 20 2017

Status: Fixed (was: Started)
Thanks for the explanation!

I'll mark this as fixed. If Alex really cares about getting this into M62 (and is willing to deign to use the issue tracker :-P), he can argue for the merge.
Labels: -Hotlist-Merge-Reject -Merge-Reject-62 Merge-Approved-62
Project Member

Comment 13 by bugdroid1@chromium.org, Oct 4 2017

Labels: -merge-approved-62 merge-merged-3202
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2added5cfbc97d7e45b21e3f86654eff361efb29

commit 2added5cfbc97d7e45b21e3f86654eff361efb29
Author: Daniel Erat <derat@chromium.org>
Date: Wed Oct 04 01:03:44 2017

chromeos: Turn display off after lock via power button.

Make ash::PowerButtonController force the backlights off
three seconds after screen locking starts when the
--force-clamshell-power-button flag is set. This avoids a
long, awkward period where the display is on with the lock
screen visible. The behavior on convertible and clamshell
devices that don't have the flag is unchanged.

TBR=derat@chromium.org

(cherry picked from commit 948c94d13ae3e5dce977c4bdaca54e4712364f93)

Bug:  758574 
Change-Id: I5a645def1205948419d7cbb711fbdbc2daaaf9e7
Reviewed-on: https://chromium-review.googlesource.com/671669
Reviewed-by: Qiang(Joe) Xu <warx@chromium.org>
Commit-Queue: Dan Erat <derat@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#502751}
Reviewed-on: https://chromium-review.googlesource.com/699566
Reviewed-by: Dan Erat <derat@chromium.org>
Cr-Commit-Position: refs/branch-heads/3202@{#574}
Cr-Branched-From: fa6a5d87adff761bc16afc5498c3f5944c1daa68-refs/heads/master@{#499098}
[modify] https://crrev.com/2added5cfbc97d7e45b21e3f86654eff361efb29/ash/wm/lock_state_controller_unittest.cc
[modify] https://crrev.com/2added5cfbc97d7e45b21e3f86654eff361efb29/ash/wm/power_button_controller.cc
[modify] https://crrev.com/2added5cfbc97d7e45b21e3f86654eff361efb29/ash/wm/power_button_controller.h
[modify] https://crrev.com/2added5cfbc97d7e45b21e3f86654eff361efb29/chromeos/dbus/fake_power_manager_client.h

Sign in to add a comment