chromeos: Save display power state on config failure.

chromeos: Save display power state on config failure.

Make DisplayConfigurator save the requested display power
state even when it fails to configure CRTCs. This lets the
previously-requested state be used for future mode changes.

BUG=chrome-os-partner:31571
Committed: https://crrev.com/4a06278cc401483a59c82927c8975a6cc85bd96f
Cr-Commit-Position: refs/heads/master@{#296798}

[Daniel Erat, 2014-09-24 23:35:58]

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

[dnicoara, 2014-09-25 13:59:55]

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

https://codereview.chromium.org/597683004/diff/1/ui/display/chromeos/display_...
ui/display/chromeos/display_configurator.cc:476: return true;
This is a bit unrelated to this CL, but should we return |success| here?

https://codereview.chromium.org/597683004/diff/1/ui/display/chromeos/display_...
File ui/display/chromeos/display_configurator_unittest.cc (right):

https://codereview.chromium.org/597683004/diff/1/ui/display/chromeos/display_...
ui/display/chromeos/display_configurator_unittest.cc:1292:
configurator_.SetDisplayPower(chromeos::DISPLAY_POWER_ALL_ON,
It makes me feel a bit uncomfortable that this doesn't check that the transition
fails. If this ever succeeds the following state will always be
DISPLAY_POWER_ALL_ON.

[Daniel Erat, 2014-09-25 17:31:06]

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

https://codereview.chromium.org/597683004/diff/1/ui/display/chromeos/display_...
ui/display/chromeos/display_configurator.cc:476: return true;
On 2014/09/25 13:59:54, dnicoara wrote:
> This is a bit unrelated to this CL, but should we return |success| here?

i'm fine with changing it. it's unclear what the return value is supposed to
mean (on top of nobody checking it); false is returned if |configure_display_|
is set, for instance. but saying something like, "Returns true if the system
successfully enters (or was already in) |power_state|." seems reasonable to me.

https://codereview.chromium.org/597683004/diff/1/ui/display/chromeos/display_...
File ui/display/chromeos/display_configurator_unittest.cc (right):

https://codereview.chromium.org/597683004/diff/1/ui/display/chromeos/display_...
ui/display/chromeos/display_configurator_unittest.cc:1292:
configurator_.SetDisplayPower(chromeos::DISPLAY_POWER_ALL_ON,
On 2014/09/25 13:59:54, dnicoara wrote:
> It makes me feel a bit uncomfortable that this doesn't check that the
transition
> fails. If this ever succeeds the following state will always be
> DISPLAY_POWER_ALL_ON.

i've added expectations for the return values to the two calls to
SetDisplayPower(). do you think that's good enough, or would you rather that the
test spelled out the actions passed to the delegate?

(is your specific concern that the stub delegate could regress such that it
reports success for this request?)

[Daniel Erat, 2014-09-25 17:32:21]

nice, hitting the spacebar after you click the "publish all my drafts" sends a
second copy of the message.

this is "do not click the 'order' button more than once; if you do, your credit
card may be charged multiple times" all over again. :-/

[dnicoara, 2014-09-25 17:50:35]

lgtm

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

https://codereview.chromium.org/597683004/diff/1/ui/display/chromeos/display_...
ui/display/chromeos/display_configurator.cc:476: return true;
On 2014/09/25 17:31:06, Daniel Erat wrote:
> On 2014/09/25 13:59:54, dnicoara wrote:
> > This is a bit unrelated to this CL, but should we return |success| here?
> 
> i'm fine with changing it. it's unclear what the return value is supposed to
> mean (on top of nobody checking it); false is returned if |configure_display_|
> is set, for instance. but saying something like, "Returns true if the system
> successfully enters (or was already in) |power_state|." seems reasonable to
me.

Thanks! This makes it more consistent with other similar calls.

https://codereview.chromium.org/597683004/diff/1/ui/display/chromeos/display_...
File ui/display/chromeos/display_configurator_unittest.cc (right):

https://codereview.chromium.org/597683004/diff/1/ui/display/chromeos/display_...
ui/display/chromeos/display_configurator_unittest.cc:1292:
configurator_.SetDisplayPower(chromeos::DISPLAY_POWER_ALL_ON,
On 2014/09/25 17:31:06, Daniel Erat wrote:
> On 2014/09/25 13:59:54, dnicoara wrote:
> > It makes me feel a bit uncomfortable that this doesn't check that the
> transition
> > fails. If this ever succeeds the following state will always be
> > DISPLAY_POWER_ALL_ON.
> 
> i've added expectations for the return values to the two calls to
> SetDisplayPower(). do you think that's good enough, or would you rather that
the
> test spelled out the actions passed to the delegate?
> 

Thank you. I think this should be fine since we're ultimately testing the state
transitions.

> (is your specific concern that the stub delegate could regress such that it
> reports success for this request?)

The stub delegate was just part of my worry. I am more worried that
DisplayConfigurator could be modified to swallow the error incorrectly.

[Daniel Erat, 2014-09-25 20:00:51]

The CQ bit was checked by derat@chromium.org

[commit-bot: I haz the power, 2014-09-25 20:02:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/597683004/20001

[commit-bot: I haz the power, 2014-09-25 21:46:32]

Committed patchset #2 (id:20001) as 4f505e51b4f561dbebf7e36f1cc4b3cbe24dbd19

[commit-bot: I haz the power, 2014-09-25 21:47:20]

Patchset 2 (id:??) landed as
https://crrev.com/4a06278cc401483a59c82927c8975a6cc85bd96f
Cr-Commit-Position: refs/heads/master@{#296798}