chromeos: Avoid shutdown for delayed power button events.

chromeos: Avoid shutdown for delayed power button events.

powerd makes blocking D-Bus calls to Chrome to turn the display on or
off. When a power-button-down event turns the display on, this can
result in the corresponding power-button-up event being delayed, which
can then cause the system to be shut down prematurely.

As a hopefully low-risk workaround for this, make
TabletPowerButtonController wait 2000 (rather than 500) milliseconds
before starting the shutdown animation if the screen was off when the
power button was pressed.

BUG=685734
Review-Url: https://codereview.chromium.org/2651313004
Cr-Commit-Position: refs/heads/master@{#446832}
Committed: https://chromium.googlesource.com/chromium/src/+/97f83d816742eef5677384925fd92d10eaa7dc9a

[Daniel Erat, 2017-01-27 20:13:54]

The CQ bit was checked by derat@chromium.org to run a CQ dry run

[Daniel Erat, 2017-01-27 20:14:08]

derat@chromium.org changed reviewers:
+ warx@chromium.org

[commit-bot: I haz the power, 2017-01-27 20:14:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Qiang(Joe) Xu, 2017-01-27 20:33:58]

On 2017/01/27 20:14:09, Daniel Erat wrote:

This is a good workaround. Helps fix for crbug.com/685734. And I have verified
that.

However, for veyron_minnie/kevin, when screen is off, it has less delay between
two power button events.

For example on veyron_minnie:
[0127/202810:INFO:display_power_setter.cc(80)] Asking Chrome to turn all
displays off
[0127/202810:INFO:internal_backlight_controller.cc(674)] Setting brightness to 0
(0%) over 0 ms
[0127/202810:INFO:daemon.cc(1341)] Chrome is using normal display mode
[0127/202813:INFO:daemon.cc(525)] Power button down
[0127/202813:INFO:main.cc(240)] Launching "sync"
[0127/202813:INFO:daemon.cc(1391)] Received request to stop forcing backlights
off
[0127/202813:INFO:display_power_setter.cc(80)] Asking Chrome to turn all
displays on
[0127/202813:INFO:internal_backlight_controller.cc(674)] Setting brightness to
94 (63%) over 0 ms
[0127/202813:INFO:daemon.cc(1341)] Chrome is using normal display mode
[0127/202813:INFO:daemon.cc(525)] Power button up

Is it ok for two timeout values for veyron_minnie? I am building veyron_minnie
right now... Could know the user experience soon...

But I believe cyan's case's priority is higher. We could ship this workaround if
you agree with my concern.

[commit-bot: I haz the power, 2017-01-27 20:54:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-27 20:54:19]

Dry run: This issue passed the CQ dry run.

[Daniel Erat, 2017-01-27 21:12:21]

On 2017/01/27 20:33:58, Qiang(Joe) Xu wrote:
> On 2017/01/27 20:14:09, Daniel Erat wrote:
> 
> This is a good workaround. Helps fix for crbug.com/685734. And I have verified
> that.
> 
> However, for veyron_minnie/kevin, when screen is off, it has less delay
between
> two power button events.
> 
> For example on veyron_minnie:
> [0127/202810:INFO:display_power_setter.cc(80)] Asking Chrome to turn all
> displays off
> [0127/202810:INFO:internal_backlight_controller.cc(674)] Setting brightness to
0
> (0%) over 0 ms
> [0127/202810:INFO:daemon.cc(1341)] Chrome is using normal display mode
> [0127/202813:INFO:daemon.cc(525)] Power button down
> [0127/202813:INFO:main.cc(240)] Launching "sync"
> [0127/202813:INFO:daemon.cc(1391)] Received request to stop forcing backlights
> off
> [0127/202813:INFO:display_power_setter.cc(80)] Asking Chrome to turn all
> displays on
> [0127/202813:INFO:internal_backlight_controller.cc(674)] Setting brightness to
> 94 (63%) over 0 ms
> [0127/202813:INFO:daemon.cc(1341)] Chrome is using normal display mode
> [0127/202813:INFO:daemon.cc(525)] Power button up
> 
> Is it ok for two timeout values for veyron_minnie? I am building veyron_minnie
> right now... Could know the user experience soon...
> 
> But I believe cyan's case's priority is higher. We could ship this workaround
if
> you agree with my concern.

thanks for testing it! i think it's okay to also increase the delay a bit on
systems that don't have the problem with button-up events being delayed: i think
that users don't have strong expectations about what's going to happen if they
hold the power button when the screen is off (and note that for non-tablet
devices, i think we completely ignore this power button press and don't lock the
screen).

with that said, if a delay shorter than 3 seconds is sufficient to work around
the problem on cyan, that sounds fine to me too. let me know if a shorter delay
seems to work just as well. thanks again! :-)

[Qiang(Joe) Xu, 2017-01-27 22:35:08]

On 2017/01/27 21:12:21, Daniel Erat wrote:
> On 2017/01/27 20:33:58, Qiang(Joe) Xu wrote:
> > On 2017/01/27 20:14:09, Daniel Erat wrote:
> > 
> > This is a good workaround. Helps fix for crbug.com/685734. And I have
verified
> > that.
> > 
> > However, for veyron_minnie/kevin, when screen is off, it has less delay
> between
> > two power button events.
> > 
> > For example on veyron_minnie:
> > [0127/202810:INFO:display_power_setter.cc(80)] Asking Chrome to turn all
> > displays off
> > [0127/202810:INFO:internal_backlight_controller.cc(674)] Setting brightness
to
> 0
> > (0%) over 0 ms
> > [0127/202810:INFO:daemon.cc(1341)] Chrome is using normal display mode
> > [0127/202813:INFO:daemon.cc(525)] Power button down
> > [0127/202813:INFO:main.cc(240)] Launching "sync"
> > [0127/202813:INFO:daemon.cc(1391)] Received request to stop forcing
backlights
> > off
> > [0127/202813:INFO:display_power_setter.cc(80)] Asking Chrome to turn all
> > displays on
> > [0127/202813:INFO:internal_backlight_controller.cc(674)] Setting brightness
to
> > 94 (63%) over 0 ms
> > [0127/202813:INFO:daemon.cc(1341)] Chrome is using normal display mode
> > [0127/202813:INFO:daemon.cc(525)] Power button up
> > 
> > Is it ok for two timeout values for veyron_minnie? I am building
veyron_minnie
> > right now... Could know the user experience soon...
> > 
> > But I believe cyan's case's priority is higher. We could ship this
workaround
> if
> > you agree with my concern.
> 
> thanks for testing it! i think it's okay to also increase the delay a bit on
> systems that don't have the problem with button-up events being delayed: i
think
> that users don't have strong expectations about what's going to happen if they
> hold the power button when the screen is off (and note that for non-tablet
> devices, i think we completely ignore this power button press and don't lock
the
> screen).
> 
> with that said, if a delay shorter than 3 seconds is sufficient to work around
> the problem on cyan, that sounds fine to me too. let me know if a shorter
delay
> seems to work just as well. thanks again! :-)

I tried 2 seconds multiple times and cannot see the shutdown animation. So I
believe two seconds is sufficient?
lgtm for the change.

[Daniel Erat, 2017-01-27 22:58:41]

Description was changed from

==========
chromeos: Avoid shutdown for delayed power button events.

powerd makes blocking D-Bus calls to Chrome to turn the display on or
off. When a power-button-down event turns the display on, this can
result in the corresponding power-button-up event being delayed, which
can then cause the system to be shut down prematurely.

As a hopefully low-risk workaround for this, make
TabletPowerButtonController wait 3000 (rather than 500) milliseconds
before starting the shutdown animation if the screen was off when the
power button was pressed.

BUG=685734
==========

to

==========
chromeos: Avoid shutdown for delayed power button events.

powerd makes blocking D-Bus calls to Chrome to turn the display on or
off. When a power-button-down event turns the display on, this can
result in the corresponding power-button-up event being delayed, which
can then cause the system to be shut down prematurely.

As a hopefully low-risk workaround for this, make
TabletPowerButtonController wait 2000 (rather than 500) milliseconds
before starting the shutdown animation if the screen was off when the
power button was pressed.

BUG=685734
==========

[Daniel Erat, 2017-01-27 22:59:01]

thanks again! i've switched it to two seconds.

[Daniel Erat, 2017-01-27 22:59:06]

The CQ bit was checked by derat@chromium.org

[Daniel Erat, 2017-01-27 22:59:07]

The patchset sent to the CQ was uploaded after l-g-t-m from warx@chromium.org
Link to the patchset: https://codereview.chromium.org/2651313004/#ps20001
(title: "switch to two seconds")

[commit-bot: I haz the power, 2017-01-27 22:59:48]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-27 23:43:07]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1485557946817520,
"parent_rev": "36b47e73e35bcea80ed02f6da9f8b3652545726c", "commit_rev":
"97f83d816742eef5677384925fd92d10eaa7dc9a"}

[commit-bot: I haz the power, 2017-01-27 23:43:41]

Description was changed from

==========
chromeos: Avoid shutdown for delayed power button events.

powerd makes blocking D-Bus calls to Chrome to turn the display on or
off. When a power-button-down event turns the display on, this can
result in the corresponding power-button-up event being delayed, which
can then cause the system to be shut down prematurely.

As a hopefully low-risk workaround for this, make
TabletPowerButtonController wait 2000 (rather than 500) milliseconds
before starting the shutdown animation if the screen was off when the
power button was pressed.

BUG=685734
==========

to

==========
chromeos: Avoid shutdown for delayed power button events.

powerd makes blocking D-Bus calls to Chrome to turn the display on or
off. When a power-button-down event turns the display on, this can
result in the corresponding power-button-up event being delayed, which
can then cause the system to be shut down prematurely.

As a hopefully low-risk workaround for this, make
TabletPowerButtonController wait 2000 (rather than 500) milliseconds
before starting the shutdown animation if the screen was off when the
power button was pressed.

BUG=685734

Review-Url: https://codereview.chromium.org/2651313004
Cr-Commit-Position: refs/heads/master@{#446832}
Committed:
https://chromium.googlesource.com/chromium/src/+/97f83d816742eef5677384925fd9...
==========

[commit-bot: I haz the power, 2017-01-27 23:43:42]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/97f83d816742eef5677384925fd9...