chromeos: Turn off displays on suspend

chromeos: Turn off displays on suspend

To handle lucid sleep (where we need to silently resume the system), turn
off the displays on suspend. This also removes the delay for restoring the
display state added in "On resume perform a delayed call to
SetDisplayPower()" According to the bug for that change, it didn't seem to
help with the issue anyways.

BUG=535021
TEST=suspend/resume of various cros platforms with/without external monitor
connected

Committed: https://crrev.com/fee6ba82e703d68c296730aa38ea1d4f77854631
Cr-Commit-Position: refs/heads/master@{#389875}

[dbasehore, 2016-04-05 05:01:10]

Description was changed from

==========
chromeos: Turn off displays on suspend

To handle lucid sleep (where we need to silently resume the system), turn
off the displays on suspend. This also removes the delay for restoring the
display state added in "On resume perform a delayed call to
SetDisplayPower()" According to the bug for that change, it didn't seem to
help with the issue anyways.

BUG=535021
TEST=suspend/resume of various cros platforms with/without external monitor
connected
==========

to

==========
chromeos: Turn off displays on suspend

To handle lucid sleep (where we need to silently resume the system), turn
off the displays on suspend. This also removes the delay for restoring the
display state added in "On resume perform a delayed call to
SetDisplayPower()" According to the bug for that change, it didn't seem to
help with the issue anyways.

BUG=535021
TEST=suspend/resume of various cros platforms with/without external monitor
connected
==========

[dbasehore, 2016-04-05 05:01:10]

dbasehore@chromium.org changed reviewers:
+ derat@chromium.org, dnicoara@chromium.org, ejcaruso@chromium.org,
marcheu@chromium.org, oshima@chromium.org, snanda@chromium.org,
tbroch@chromium.org

[dbasehore, 2016-04-05 05:01:55]

display unittests now pass.

[Daniel Erat, 2016-04-05 14:40:04]

https://codereview.chromium.org/1861593002/diff/1/ui/display/chromeos/display...
File ui/display/chromeos/display_configurator.cc (right):

https://codereview.chromium.org/1861593002/diff/1/ui/display/chromeos/display...
ui/display/chromeos/display_configurator.cc:952: saved_power_state_ =
chromeos::DISPLAY_POWER_ALL_OFF;
what's the reason for resetting this here? doesn't it always get updated before
suspending in SuspendDisplays?

https://codereview.chromium.org/1861593002/diff/1/ui/display/chromeos/display...
ui/display/chromeos/display_configurator.cc:953: if (power_state ==
chromeos::DISPLAY_POWER_ALL_OFF)
add curly brackets for this if/else

https://codereview.chromium.org/1861593002/diff/1/ui/display/chromeos/display...
ui/display/chromeos/display_configurator.cc:954:
SetDisplayPower(chromeos::DISPLAY_POWER_ALL_ON,
what happens if the user sets the brightness to zero and then the system
suspends and resumes? doesn't this undo what the user asked for?

[dnicoara, 2016-04-05 16:56:57]

https://codereview.chromium.org/1861593002/diff/1/ui/display/chromeos/display...
File ui/display/chromeos/display_configurator.cc (right):

https://codereview.chromium.org/1861593002/diff/1/ui/display/chromeos/display...
ui/display/chromeos/display_configurator.cc:955:
kSetDisplayPowerOnlyIfSingleInternalDisplay,
I think you still want kSetDisplayPowerForceProbe regardless of the power state.
On resume we need to force a probe since there may have been display events
(unplugged/plugged displays).

[Eric Caruso, 2016-04-05 17:33:36]

https://codereview.chromium.org/1861593002/diff/1/ui/display/chromeos/display...
File ui/display/chromeos/display_configurator.cc (right):

https://codereview.chromium.org/1861593002/diff/1/ui/display/chromeos/display...
ui/display/chromeos/display_configurator.cc:940: // completes before we return,
because otherwise the X message could be
Aren't we not using X anymore?

[dbasehore, 2016-04-06 04:40:40]

https://codereview.chromium.org/1861593002/diff/1/ui/display/chromeos/display...
File ui/display/chromeos/display_configurator.cc (right):

https://codereview.chromium.org/1861593002/diff/1/ui/display/chromeos/display...
ui/display/chromeos/display_configurator.cc:940: // completes before we return,
because otherwise the X message could be
On 2016/04/05 17:33:36, Eric Caruso wrote:
> Aren't we not using X anymore?

Done.

https://codereview.chromium.org/1861593002/diff/1/ui/display/chromeos/display...
ui/display/chromeos/display_configurator.cc:952: saved_power_state_ =
chromeos::DISPLAY_POWER_ALL_OFF;
On 2016/04/05 14:40:03, Daniel Erat wrote:
> what's the reason for resetting this here? doesn't it always get updated
before
> suspending in SuspendDisplays?

Done.

https://codereview.chromium.org/1861593002/diff/1/ui/display/chromeos/display...
ui/display/chromeos/display_configurator.cc:953: if (power_state ==
chromeos::DISPLAY_POWER_ALL_OFF)
On 2016/04/05 14:40:04, Daniel Erat wrote:
> add curly brackets for this if/else

Done.

https://codereview.chromium.org/1861593002/diff/1/ui/display/chromeos/display...
ui/display/chromeos/display_configurator.cc:954:
SetDisplayPower(chromeos::DISPLAY_POWER_ALL_ON,
On 2016/04/05 14:40:03, Daniel Erat wrote:
> what happens if the user sets the brightness to zero and then the system
> suspends and resumes? doesn't this undo what the user asked for?

Don't know how, but it doesn't undo what the user set.

We do something similar right now by always turning on the display during
suspend if it's just a single display.

https://codereview.chromium.org/1861593002/diff/1/ui/display/chromeos/display...
ui/display/chromeos/display_configurator.cc:955:
kSetDisplayPowerOnlyIfSingleInternalDisplay,
On 2016/04/05 16:56:57, dnicoara wrote:
> I think you still want kSetDisplayPowerForceProbe regardless of the power
state.
> On resume we need to force a probe since there may have been display events
> (unplugged/plugged displays).

I'm changing how this works for the next version.

[Daniel Erat, 2016-04-06 15:04:19]

https://codereview.chromium.org/1861593002/diff/1/ui/display/chromeos/display...
File ui/display/chromeos/display_configurator.cc (right):

https://codereview.chromium.org/1861593002/diff/1/ui/display/chromeos/display...
ui/display/chromeos/display_configurator.cc:954:
SetDisplayPower(chromeos::DISPLAY_POWER_ALL_ON,
On 2016/04/06 04:40:40, dbasehore wrote:
> On 2016/04/05 14:40:03, Daniel Erat wrote:
> > what happens if the user sets the brightness to zero and then the system
> > suspends and resumes? doesn't this undo what the user asked for?
> 
> Don't know how, but it doesn't undo what the user set.
> 
> We do something similar right now by always turning on the display during
> suspend if it's just a single display.

i'm uneasy not knowing how/why this works. so you're saying that with this
change, if you use the brightness-down key to reduce to 0% (and also turn the
display off), and then suspend/resume, the display is still off instead of
getting turned back on? does powerd send another message post-resume requesting
that the displays be turned off?

[marcheu1, 2016-04-06 20:21:39]

On 2016/04/06 15:04:19, Daniel Erat wrote:
>
https://codereview.chromium.org/1861593002/diff/1/ui/display/chromeos/display...
> File ui/display/chromeos/display_configurator.cc (right):
> 
>
https://codereview.chromium.org/1861593002/diff/1/ui/display/chromeos/display...
> ui/display/chromeos/display_configurator.cc:954:
> SetDisplayPower(chromeos::DISPLAY_POWER_ALL_ON,
> On 2016/04/06 04:40:40, dbasehore wrote:
> > On 2016/04/05 14:40:03, Daniel Erat wrote:
> > > what happens if the user sets the brightness to zero and then the system
> > > suspends and resumes? doesn't this undo what the user asked for?
> > 
> > Don't know how, but it doesn't undo what the user set.

Is it because of the backlight resume brightness changes maybe? We could test
without it, we plan to disable eventually.

> > 
> > We do something similar right now by always turning on the display during
> > suspend if it's just a single display.
> 
> i'm uneasy not knowing how/why this works. so you're saying that with this
> change, if you use the brightness-down key to reduce to 0% (and also turn the
> display off), and then suspend/resume, the display is still off instead of
> getting turned back on? does powerd send another message post-resume
requesting
> that the displays be turned off?

Also note that we are being hit by this:
https://bugs.chromium.org/p/chromium/issues/detail?id=597131

If we decide to revert I don't really want to go on a revert spree in multiple
places. So 597131 needs to be sorted out first.

[marcheu1, 2016-04-06 20:21:49]

marcheu@google.com changed reviewers:
+ marcheu@google.com

[marcheu1, 2016-04-06 20:21:50]

https://codereview.chromium.org/1861593002/diff/20001/ui/display/chromeos/dis...
File ui/display/chromeos/display_configurator.cc (left):

https://codereview.chromium.org/1861593002/diff/20001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator.cc:962: base::Unretained(this)));
I think this is still needed. The kernel makes no guarantee (and as a matter of
fact it doesn't always work in practice) that you will get a hotplug event if
the monitor configuration changes while you were suspended (for example if you
added or removed a monitor).

[dbasehore, 2016-04-06 20:44:45]

https://codereview.chromium.org/1861593002/diff/1/ui/display/chromeos/display...
File ui/display/chromeos/display_configurator.cc (right):

https://codereview.chromium.org/1861593002/diff/1/ui/display/chromeos/display...
ui/display/chromeos/display_configurator.cc:954:
SetDisplayPower(chromeos::DISPLAY_POWER_ALL_ON,
On 2016/04/06 15:04:19, Daniel Erat wrote:
> On 2016/04/06 04:40:40, dbasehore wrote:
> > On 2016/04/05 14:40:03, Daniel Erat wrote:
> > > what happens if the user sets the brightness to zero and then the system
> > > suspends and resumes? doesn't this undo what the user asked for?
> > 
> > Don't know how, but it doesn't undo what the user set.
> > 
> > We do something similar right now by always turning on the display during
> > suspend if it's just a single display.
> 
> i'm uneasy not knowing how/why this works. so you're saying that with this
> change, if you use the brightness-down key to reduce to 0% (and also turn the
> display off), and then suspend/resume, the display is still off instead of
> getting turned back on? does powerd send another message post-resume
requesting
> that the displays be turned off?

This seems to be how it works:
-Backlight set to 0 by user
-resume brightness set the 0 (since GetUndimmedBrightnessPercent in powerd
returns the current brightness of 0)
-System suspends (display turned off)
-System resumes (brightness set to 0 by resume brightness)
-Chrome "turns on" display, but brightness is still 0

This is also what happens *right now* since we turn on the display for a single
display on suspend. The reason that it turns on the display if it was turned off
for idle suspend is that GetUndimmedBrightnessPercent does not return 0 in that
case.

[dbasehore, 2016-04-06 20:44:45]

https://codereview.chromium.org/1861593002/diff/1/ui/display/chromeos/display...
File ui/display/chromeos/display_configurator.cc (right):

https://codereview.chromium.org/1861593002/diff/1/ui/display/chromeos/display...
ui/display/chromeos/display_configurator.cc:954:
SetDisplayPower(chromeos::DISPLAY_POWER_ALL_ON,
On 2016/04/06 15:04:19, Daniel Erat wrote:
> On 2016/04/06 04:40:40, dbasehore wrote:
> > On 2016/04/05 14:40:03, Daniel Erat wrote:
> > > what happens if the user sets the brightness to zero and then the system
> > > suspends and resumes? doesn't this undo what the user asked for?
> > 
> > Don't know how, but it doesn't undo what the user set.
> > 
> > We do something similar right now by always turning on the display during
> > suspend if it's just a single display.
> 
> i'm uneasy not knowing how/why this works. so you're saying that with this
> change, if you use the brightness-down key to reduce to 0% (and also turn the
> display off), and then suspend/resume, the display is still off instead of
> getting turned back on? does powerd send another message post-resume
requesting
> that the displays be turned off?

This seems to be how it works:
-Backlight set to 0 by user
-resume brightness set the 0 (since GetUndimmedBrightnessPercent in powerd
returns the current brightness of 0)
-System suspends (display turned off)
-System resumes (brightness set to 0 by resume brightness)
-Chrome "turns on" display, but brightness is still 0

This is also what happens *right now* since we turn on the display for a single
display on suspend. The reason that it turns on the display if it was turned off
for idle suspend is that GetUndimmedBrightnessPercent does not return 0 in that
case.

[dbasehore, 2016-04-06 20:48:41]

https://codereview.chromium.org/1861593002/diff/20001/ui/display/chromeos/dis...
File ui/display/chromeos/display_configurator.cc (left):

https://codereview.chromium.org/1861593002/diff/20001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator.cc:962: base::Unretained(this)));
On 2016/04/06 20:21:50, marcheu1 wrote:
> I think this is still needed. The kernel makes no guarantee (and as a matter
of
> fact it doesn't always work in practice) that you will get a hotplug event if
> the monitor configuration changes while you were suspended (for example if you
> added or removed a monitor).

We're just doing this in the ResumeDisplays function. Do you mean we need to
start the timer to run ConfigureDisplays?

[dbasehore, 2016-04-06 20:53:26]

https://codereview.chromium.org/1861593002/diff/1/ui/display/chromeos/display...
File ui/display/chromeos/display_configurator.cc (right):

https://codereview.chromium.org/1861593002/diff/1/ui/display/chromeos/display...
ui/display/chromeos/display_configurator.cc:954:
SetDisplayPower(chromeos::DISPLAY_POWER_ALL_ON,
On 2016/04/06 20:44:45, dbasehore wrote:
> On 2016/04/06 15:04:19, Daniel Erat wrote:
> > On 2016/04/06 04:40:40, dbasehore wrote:
> > > On 2016/04/05 14:40:03, Daniel Erat wrote:
> > > > what happens if the user sets the brightness to zero and then the system
> > > > suspends and resumes? doesn't this undo what the user asked for?
> > > 
> > > Don't know how, but it doesn't undo what the user set.
> > > 
> > > We do something similar right now by always turning on the display during
> > > suspend if it's just a single display.
> > 
> > i'm uneasy not knowing how/why this works. so you're saying that with this
> > change, if you use the brightness-down key to reduce to 0% (and also turn
the
> > display off), and then suspend/resume, the display is still off instead of
> > getting turned back on? does powerd send another message post-resume
> requesting
> > that the displays be turned off?
> 
> This seems to be how it works:
> -Backlight set to 0 by user
> -resume brightness set the 0 (since GetUndimmedBrightnessPercent in powerd
> returns the current brightness of 0)
> -System suspends (display turned off)
> -System resumes (brightness set to 0 by resume brightness)
> -Chrome "turns on" display, but brightness is still 0
> 
> This is also what happens *right now* since we turn on the display for a
single
> display on suspend. The reason that it turns on the display if it was turned
off
> for idle suspend is that GetUndimmedBrightnessPercent does not return 0 in
that
> case.

When we update the backlight state in powerd, we set the display power.  It
seems this happens no matter what on resume, but I can double check.

[dbasehore, 2016-04-08 20:15:36]

Anyone have any additional concerns?

This patch is needed to have correct panel timings on resume now that we had to
revert the other patches that fixed that
https://chromium-review.googlesource.com/#/c/337590/2

[Daniel Erat, 2016-04-08 20:26:51]

https://codereview.chromium.org/1861593002/diff/20001/ui/display/chromeos/dis...
File ui/display/chromeos/display_configurator.h (right):

https://codereview.chromium.org/1861593002/diff/20001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator.h:400: chromeos::DisplayPowerState
saved_power_state_;
here's a possibly-naive question: why is this needed? it looks like
requested_power_state_ gets copied to saved_power_state_ just before the system
suspends, and then both of them get checked on resume. is this present to handle
the case where powerd sends another request during dark resume? if so, could you
document that here?

i *think* that what's going on is that SetDisplayPower() (which is usually
called by powerd) updates requested_power_state_, and you want to save the
actually-requested-by-powerd state before you call SetDisplayPower() manually so
you can restore the state later. if that's correct, please just create a new
SetDisplayPowerInternal() method. powerd can call SetDisplayPower(), which will
update the requested state and call SetDisplayPowerInternal(), while your change
can just call SetDisplayPowerInternal() directly.

[dbasehore, 2016-04-08 20:38:11]

https://codereview.chromium.org/1861593002/diff/20001/ui/display/chromeos/dis...
File ui/display/chromeos/display_configurator.h (right):

https://codereview.chromium.org/1861593002/diff/20001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator.h:400: chromeos::DisplayPowerState
saved_power_state_;
On 2016/04/08 20:26:51, Daniel Erat wrote:
> here's a possibly-naive question: why is this needed? it looks like
> requested_power_state_ gets copied to saved_power_state_ just before the
system
> suspends, and then both of them get checked on resume. is this present to
handle
> the case where powerd sends another request during dark resume? if so, could
you
> document that here?
> 
> i *think* that what's going on is that SetDisplayPower() (which is usually
> called by powerd) updates requested_power_state_, and you want to save the
> actually-requested-by-powerd state before you call SetDisplayPower() manually
so
> you can restore the state later. if that's correct, please just create a new
> SetDisplayPowerInternal() method. powerd can call SetDisplayPower(), which
will
> update the requested state and call SetDisplayPowerInternal(), while your
change
> can just call SetDisplayPowerInternal() directly.

This is to handle the display state changing out from under the display
configurator (whether from powerd or some other place). I'll document this.

I assume SetDisplayPowerInternal will be called from the display configurator to
turn off the displays (and not save the off state in requested_power_state_)?

[Daniel Erat, 2016-04-08 20:42:11]

https://codereview.chromium.org/1861593002/diff/20001/ui/display/chromeos/dis...
File ui/display/chromeos/display_configurator.h (right):

https://codereview.chromium.org/1861593002/diff/20001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator.h:400: chromeos::DisplayPowerState
saved_power_state_;
On 2016/04/08 20:38:11, dbasehore wrote:
> On 2016/04/08 20:26:51, Daniel Erat wrote:
> > here's a possibly-naive question: why is this needed? it looks like
> > requested_power_state_ gets copied to saved_power_state_ just before the
> system
> > suspends, and then both of them get checked on resume. is this present to
> handle
> > the case where powerd sends another request during dark resume? if so, could
> you
> > document that here?
> > 
> > i *think* that what's going on is that SetDisplayPower() (which is usually
> > called by powerd) updates requested_power_state_, and you want to save the
> > actually-requested-by-powerd state before you call SetDisplayPower()
manually
> so
> > you can restore the state later. if that's correct, please just create a new
> > SetDisplayPowerInternal() method. powerd can call SetDisplayPower(), which
> will
> > update the requested state and call SetDisplayPowerInternal(), while your
> change
> > can just call SetDisplayPowerInternal() directly.
> 
> This is to handle the display state changing out from under the display
> configurator (whether from powerd or some other place). I'll document this.
> 
> I assume SetDisplayPowerInternal will be called from the display configurator
to
> turn off the displays (and not save the off state in requested_power_state_)?

yeah, that's the idea. then we don't need to track an extra state --
requested_power_state_ contains the state that powerd asked for, and that's it.
the display can be configured differently for suspend, but we know what we want
to go back to after resuming.

[dbasehore, 2016-04-12 03:44:42]

Okay, to do that, I'll have to break apart the RunPendingConfiguration task
since that just grabs requested_power_state_.

I could also change the requested_power_state_ then change it back. The value is
pulled before SetDisplayPower returns.

[Daniel Erat, 2016-04-12 14:06:12]

On 2016/04/12 03:44:42, dbasehore wrote:
> Okay, to do that, I'll have to break apart the RunPendingConfiguration task
> since that just grabs requested_power_state_.
> 
> I could also change the requested_power_state_ then change it back. The value
is
> pulled before SetDisplayPower returns.

i think it'd be clearest if requested_power_state_ always has a single meaning,
e.g. "the state we should be in if we aren't suspending or doing something
weird".

[dbasehore, 2016-04-21 21:53:51]

On 2016/04/12 14:06:12, Daniel Erat wrote:
> On 2016/04/12 03:44:42, dbasehore wrote:
> > Okay, to do that, I'll have to break apart the RunPendingConfiguration task
> > since that just grabs requested_power_state_.
> > 
> > I could also change the requested_power_state_ then change it back. The
value
> is
> > pulled before SetDisplayPower returns.
> 
> i think it'd be clearest if requested_power_state_ always has a single
meaning,
> e.g. "the state we should be in if we aren't suspending or doing something
> weird".

Addressed, even though I kept an additional variable, it tracks what we use in
the configuration task. Now requested_power_state_ tracks what is passed as a
arg to SetDisplayPower.

[Daniel Erat, 2016-04-21 22:16:42]

sorry, i still have some questions about this.

https://codereview.chromium.org/1861593002/diff/40001/ui/display/chromeos/dis...
File ui/display/chromeos/display_configurator.cc (right):

https://codereview.chromium.org/1861593002/diff/40001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator.cc:960: if (requested_power_state_ !=
chromeos::DISPLAY_POWER_ALL_OFF) {
can you explain this to me again (and also update this comment to explain it,
instead of just describing what the next two lines do :-P)? i didn't fully
understand it the previous time. if the user manually set the brightness to zero
before suspending, isn't that getting dropped here?

https://codereview.chromium.org/1861593002/diff/40001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator.cc:967: SetDisplayPower(power_state,
kSetDisplayPowerForceProbe,
it seems weird to call the public method here. if you really want to update
requested_power_state_, why not just do that in an else block above instead of
by calling the public function for something that wasn't a user request?

https://codereview.chromium.org/1861593002/diff/40001/ui/display/chromeos/dis...
File ui/display/chromeos/display_configurator.h (right):

https://codereview.chromium.org/1861593002/diff/40001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator.h:305: const ConfigurationCallback&
callback);
nit: add a blank line after this line

https://codereview.chromium.org/1861593002/diff/40001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator.h:398: chromeos::DisplayPowerState
config_power_state_;
nit: configured_power_state_? maybe current_power_state_ is even clearer?

https://codereview.chromium.org/1861593002/diff/40001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator.h:401: bool power_state_change_;
please give this variable a better name. the comment could also be improved; i
don't know what it's trying to say. what's a "user request"? it looks like this
gets set in the new internal method, so it doesn't seem like it's limited to
user-requested changes.

https://codereview.chromium.org/1861593002/diff/40001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator.h:404: int config_power_flags_;
make this either "configured" or "current" too to match

[dbasehore, 2016-04-21 22:35:10]

https://codereview.chromium.org/1861593002/diff/40001/ui/display/chromeos/dis...
File ui/display/chromeos/display_configurator.cc (right):

https://codereview.chromium.org/1861593002/diff/40001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator.cc:960: if (requested_power_state_ !=
chromeos::DISPLAY_POWER_ALL_OFF) {
On 2016/04/21 22:16:41, Daniel Erat wrote:
> can you explain this to me again (and also update this comment to explain it,
> instead of just describing what the next two lines do :-P)? i didn't fully
> understand it the previous time. if the user manually set the brightness to
zero
> before suspending, isn't that getting dropped here?

Might be able to get rid of this. If we call SetDisplayPower in powerd when we
turn on the backlight, that may have similar latency to setting it here. We turn
the backlight back on in powerd right after announcing resume. We may have to
resume the backlight before un-muting the audio in powerd since the audio call
blocks, though.

I'll remove if it turns out to work well.

https://codereview.chromium.org/1861593002/diff/40001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator.cc:967: SetDisplayPower(power_state,
kSetDisplayPowerForceProbe,
On 2016/04/21 22:16:41, Daniel Erat wrote:
> it seems weird to call the public method here. if you really want to update
> requested_power_state_, why not just do that in an else block above instead of
> by calling the public function for something that wasn't a user request?

Works for me. Will do.

https://codereview.chromium.org/1861593002/diff/40001/ui/display/chromeos/dis...
File ui/display/chromeos/display_configurator.h (right):

https://codereview.chromium.org/1861593002/diff/40001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator.h:305: const ConfigurationCallback&
callback);
On 2016/04/21 22:16:41, Daniel Erat wrote:
> nit: add a blank line after this line

Done.

https://codereview.chromium.org/1861593002/diff/40001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator.h:398: chromeos::DisplayPowerState
config_power_state_;
On 2016/04/21 22:16:41, Daniel Erat wrote:
> nit: configured_power_state_? maybe current_power_state_ is even clearer?

current_power_state already exists. configured_power_state_ implies that we
already ran the configuration. pending_power_state_?

https://codereview.chromium.org/1861593002/diff/40001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator.h:401: bool power_state_change_;
On 2016/04/21 22:16:41, Daniel Erat wrote:
> please give this variable a better name. the comment could also be improved; i
> don't know what it's trying to say. what's a "user request"? it looks like
this
> gets set in the new internal method, so it doesn't seem like it's limited to
> user-requested changes.

The name still seems to be relevant (the power state we are going to configure
with has changed). I'll update the comment. Could be pending_power_state_change_
if we go with the change above.

https://codereview.chromium.org/1861593002/diff/40001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator.h:404: int config_power_flags_;
On 2016/04/21 22:16:41, Daniel Erat wrote:
> make this either "configured" or "current" too to match

As before. Those names imply that it was already done when we actually haven't
run the configuration task yet.

[Daniel Erat, 2016-04-22 21:37:33]

thanks! lgtm with some nits

https://codereview.chromium.org/1861593002/diff/80001/ui/display/chromeos/dis...
File ui/display/chromeos/display_configurator.h (right):

https://codereview.chromium.org/1861593002/diff/80001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator.h:303: void
SetDisplayPowerInternal(chromeos::DisplayPowerState power_state,
add a comment here. maybe something like:

  // Updates |pending_*| members and applies the passed-in state.
  // |callback| is invoked (perhaps synchronously) on completion.

https://codereview.chromium.org/1861593002/diff/80001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator.h:305: const ConfigurationCallback&
callback);
nit: add a blank line after this one

https://codereview.chromium.org/1861593002/diff/80001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator.h:396: // Stores the power state to
configure. May be the requested_power_state_ or
nit: maybe change this to "The power state to be applied by
RunPendingConfiguration()." also change to |requested_power_state_| to match
other comments

https://codereview.chromium.org/1861593002/diff/80001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator.h:401: bool
pending_power_state_change_;
mind renaming this to something like |have_pending_power_state_|?

[dbasehore, 2016-04-22 22:18:57]

https://codereview.chromium.org/1861593002/diff/80001/ui/display/chromeos/dis...
File ui/display/chromeos/display_configurator.h (right):

https://codereview.chromium.org/1861593002/diff/80001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator.h:303: void
SetDisplayPowerInternal(chromeos::DisplayPowerState power_state,
On 2016/04/22 21:37:33, Daniel Erat wrote:
> add a comment here. maybe something like:
> 
>   // Updates |pending_*| members and applies the passed-in state.
>   // |callback| is invoked (perhaps synchronously) on completion.

Done.

https://codereview.chromium.org/1861593002/diff/80001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator.h:305: const ConfigurationCallback&
callback);
On 2016/04/22 21:37:33, Daniel Erat wrote:
> nit: add a blank line after this one

Done.

https://codereview.chromium.org/1861593002/diff/80001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator.h:396: // Stores the power state to
configure. May be the requested_power_state_ or
On 2016/04/22 21:37:33, Daniel Erat wrote:
> nit: maybe change this to "The power state to be applied by
> RunPendingConfiguration()." also change to |requested_power_state_| to match
> other comments

Done.

https://codereview.chromium.org/1861593002/diff/80001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator.h:401: bool
pending_power_state_change_;
On 2016/04/22 21:37:33, Daniel Erat wrote:
> mind renaming this to something like |have_pending_power_state_|?

Changed to has_pending_power_state_ since third person singular is more common
in the code.

[Daniel Erat, 2016-04-22 22:27:50]

thanks, still lgtm

https://codereview.chromium.org/1861593002/diff/120001/ui/display/chromeos/di...
File ui/display/chromeos/display_configurator.h (right):

https://codereview.chromium.org/1861593002/diff/120001/ui/display/chromeos/di...
ui/display/chromeos/display_configurator.h:304: // invoke (perhaps
synchronously) on completion.
nit: s/invoke/invoked/

[Daniel Erat, 2016-04-25 21:10:00]

https://codereview.chromium.org/1861593002/diff/160001/ui/display/chromeos/di...
File ui/display/chromeos/display_configurator_unittest.cc (right):

https://codereview.chromium.org/1861593002/diff/160001/ui/display/chromeos/di...
ui/display/chromeos/display_configurator_unittest.cc:920: // suspended.
this comment is a bit off, right? i don't think the displays are suspended here:
the display power is turned off first, and then the system suspends.

[dbasehore, 2016-04-25 21:13:23]

https://codereview.chromium.org/1861593002/diff/160001/ui/display/chromeos/di...
File ui/display/chromeos/display_configurator_unittest.cc (right):

https://codereview.chromium.org/1861593002/diff/160001/ui/display/chromeos/di...
ui/display/chromeos/display_configurator_unittest.cc:920: // suspended.
On 2016/04/25 21:10:00, Daniel Erat wrote:
> this comment is a bit off, right? i don't think the displays are suspended
here:
> the display power is turned off first, and then the system suspends.

Removed the above comment too since it doesn't make much sense either.

[Daniel Erat, 2016-04-25 21:19:22]

lgtm

[dbasehore, 2016-04-25 21:20:01]

The CQ bit was checked by dbasehore@chromium.org

[commit-bot: I haz the power, 2016-04-25 21:20:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1861593002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1861593002/180001

[commit-bot: I haz the power, 2016-04-25 22:22:01]

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

[commit-bot: I haz the power, 2016-04-25 22:22:03]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dbasehore, 2016-04-26 00:41:51]

On 2016/04/25 22:22:03, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux
(JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

The issue with ash_unittests should be fixed now.

[dbasehore, 2016-04-26 00:55:18]

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

[commit-bot: I haz the power, 2016-04-26 00:55:47]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1861593002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1861593002/200001

[dbasehore, 2016-04-26 01:23:18]

On 2016/04/26 00:55:47, commit-bot: I haz the power wrote:
> Dry run: CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/1861593002/200001
> View timeline at
>  https://chromium-cq-status.appspot.com/patch-timeline/1861593002/200001

linux_chromium_chromeos_ozone_rel_ng (which was failing) now passes with this
CL.

[commit-bot: I haz the power, 2016-04-26 01:50:07]

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

[commit-bot: I haz the power, 2016-04-26 01:50:08]

Dry run: This issue passed the CQ dry run.

[Daniel Erat, 2016-04-26 15:14:25]

lgtm

[dbasehore, 2016-04-26 20:39:42]

The CQ bit was checked by dbasehore@chromium.org

[commit-bot: I haz the power, 2016-04-26 20:40:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1861593002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1861593002/200001

[commit-bot: I haz the power, 2016-04-26 20:44:54]

Description was changed from

==========
chromeos: Turn off displays on suspend

To handle lucid sleep (where we need to silently resume the system), turn
off the displays on suspend. This also removes the delay for restoring the
display state added in "On resume perform a delayed call to
SetDisplayPower()" According to the bug for that change, it didn't seem to
help with the issue anyways.

BUG=535021
TEST=suspend/resume of various cros platforms with/without external monitor
connected
==========

to

==========
chromeos: Turn off displays on suspend

To handle lucid sleep (where we need to silently resume the system), turn
off the displays on suspend. This also removes the delay for restoring the
display state added in "On resume perform a delayed call to
SetDisplayPower()" According to the bug for that change, it didn't seem to
help with the issue anyways.

BUG=535021
TEST=suspend/resume of various cros platforms with/without external monitor
connected
==========

[commit-bot: I haz the power, 2016-04-26 20:44:55]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-04-26 20:46:25]

Description was changed from

==========
chromeos: Turn off displays on suspend

To handle lucid sleep (where we need to silently resume the system), turn
off the displays on suspend. This also removes the delay for restoring the
display state added in "On resume perform a delayed call to
SetDisplayPower()" According to the bug for that change, it didn't seem to
help with the issue anyways.

BUG=535021
TEST=suspend/resume of various cros platforms with/without external monitor
connected
==========

to

==========
chromeos: Turn off displays on suspend

To handle lucid sleep (where we need to silently resume the system), turn
off the displays on suspend. This also removes the delay for restoring the
display state added in "On resume perform a delayed call to
SetDisplayPower()" According to the bug for that change, it didn't seem to
help with the issue anyways.

BUG=535021
TEST=suspend/resume of various cros platforms with/without external monitor
connected

Committed: https://crrev.com/fee6ba82e703d68c296730aa38ea1d4f77854631
Cr-Commit-Position: refs/heads/master@{#389875}
==========

[commit-bot: I haz the power, 2016-04-26 20:46:27]

Patchset 11 (id:??) landed as
https://crrev.com/fee6ba82e703d68c296730aa38ea1d4f77854631
Cr-Commit-Position: refs/heads/master@{#389875}

[dbasehore, 2016-04-26 23:50:44]

Description was changed from

==========
chromeos: Turn off displays on suspend

To handle lucid sleep (where we need to silently resume the system), turn
off the displays on suspend. This also removes the delay for restoring the
display state added in "On resume perform a delayed call to
SetDisplayPower()" According to the bug for that change, it didn't seem to
help with the issue anyways.

BUG=535021
TEST=suspend/resume of various cros platforms with/without external monitor
connected

Committed: https://crrev.com/fee6ba82e703d68c296730aa38ea1d4f77854631
Cr-Commit-Position: refs/heads/master@{#389875}
==========

to

==========
chromeos: Turn off displays on suspend

To handle lucid sleep (where we need to silently resume the system), turn
off the displays on suspend. This also removes the delay for restoring the
display state added in "On resume perform a delayed call to
SetDisplayPower()" According to the bug for that change, it didn't seem to
help with the issue anyways.

BUG=535021
TEST=suspend/resume of various cros platforms with/without external monitor
connected

Committed: https://crrev.com/fee6ba82e703d68c296730aa38ea1d4f77854631
Cr-Commit-Position: refs/heads/master@{#389875}
==========

[dbasehore, 2016-04-26 23:50:44]

dbasehore@chromium.org changed reviewers:
+ spang@chromium.org

[Daniel Kurtz, 2016-05-05 10:44:57]

A revert of this CL (patchset #12 id:220001) has been created in
https://codereview.chromium.org/1949753004/ by djkurtz@chromium.org.

The reason for reverting is: With this patch, the internal display (on oak, at
least) stays black on resume from suspend.

Perhaps "the delay for restoring the display state" really is important after
all.  Next time, I recommend including just one logical change per patch.

BUG=chrome-os-partner:52916

.