Make SetDisplayPower() take a callback to signal completion

Make SetDisplayPower() take a callback to signal completion

SetDisplayPower() is called by powerd to turn the panel on. After that
powerd will set the backlight level. Since DisplayConfigurator may be
executing the SetDisplayPower() operation asynchronously, the panel
may still be off when powerd tries to set the backlight level. So
powerd's operation would be ignored by the driver.

BUG=chrome-os-partner:35662
Committed: https://crrev.com/eebe273b953b5c726473d2af63feb13dd0d5f090
Cr-Commit-Position: refs/heads/master@{#313983}

[dnicoara, 2015-01-30 16:48:49]

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

[dnicoara, 2015-01-30 16:48:50]

PTAL

[Daniel Erat, 2015-01-30 17:14:07]

looks okay to me with some minor comments

https://codereview.chromium.org/886103002/diff/20001/chromeos/dbus/services/d...
File chromeos/dbus/services/display_power_service_provider.cc (right):

https://codereview.chromium.org/886103002/diff/20001/chromeos/dbus/services/d...
chromeos/dbus/services/display_power_service_provider.cc:15: void
DisplayConfigurationCallback(
nit: put a verb in this (RunConfigurationCallback()?)

https://codereview.chromium.org/886103002/diff/20001/chromeos/dbus/services/d...
File chromeos/dbus/services/display_power_service_provider.h (right):

https://codereview.chromium.org/886103002/diff/20001/chromeos/dbus/services/d...
chromeos/dbus/services/display_power_service_provider.h:38: // Sets the display
power state. After the display power is set, callback
nit: s/callback/|callback|/

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

https://codereview.chromium.org/886103002/diff/20001/ui/display/chromeos/disp...
ui/display/chromeos/display_configurator.h:383: // start/finish. Note these
callbacks belong to pending request not a
the use of "pending" here is pretty confusing given the
|pending_configuration_callbacks_| member. :-) maybe rename
|pending_configuration_callbacks_| to |active_configuration_callbacks_| or
|in_progress_configuration_callbacks_| to make the distinction clearer?

also nit: add comma after "request"

https://codereview.chromium.org/886103002/diff/20001/ui/display/chromeos/disp...
File ui/display/chromeos/display_configurator_unittest.cc (right):

https://codereview.chromium.org/886103002/diff/20001/ui/display/chromeos/disp...
ui/display/chromeos/display_configurator_unittest.cc:453:
CheckAndResetCallback(true);
CheckAndResetCallback() failures won't produce meaningful line numbers or stack
traces, will they? how about using an enum instead?

  enum CallbackResult {
    CALLBACK_SUCCESS,
    CALLBACK_FAILURE,
    CALLBACK_NOT_CALLED,
  }

  CallbackResult PopCallbackResult() {
    // choose enum value, clear the "called" field, and return
  }

and then in the tests:

  EXPECT_EQ(CALLBACK_SUCCESS, PopCallbackResult());

[dnicoara, 2015-01-30 18:25:28]

https://codereview.chromium.org/886103002/diff/20001/chromeos/dbus/services/d...
File chromeos/dbus/services/display_power_service_provider.cc (right):

https://codereview.chromium.org/886103002/diff/20001/chromeos/dbus/services/d...
chromeos/dbus/services/display_power_service_provider.cc:15: void
DisplayConfigurationCallback(
On 2015/01/30 17:14:07, Daniel Erat wrote:
> nit: put a verb in this (RunConfigurationCallback()?)

Done.

https://codereview.chromium.org/886103002/diff/20001/chromeos/dbus/services/d...
File chromeos/dbus/services/display_power_service_provider.h (right):

https://codereview.chromium.org/886103002/diff/20001/chromeos/dbus/services/d...
chromeos/dbus/services/display_power_service_provider.h:38: // Sets the display
power state. After the display power is set, callback
On 2015/01/30 17:14:07, Daniel Erat wrote:
> nit: s/callback/|callback|/

Done.

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

https://codereview.chromium.org/886103002/diff/20001/ui/display/chromeos/disp...
ui/display/chromeos/display_configurator.h:383: // start/finish. Note these
callbacks belong to pending request not a
On 2015/01/30 17:14:07, Daniel Erat wrote:
> the use of "pending" here is pretty confusing given the
> |pending_configuration_callbacks_| member. :-) maybe rename
> |pending_configuration_callbacks_| to |active_configuration_callbacks_| or
> |in_progress_configuration_callbacks_| to make the distinction clearer?
> 
> also nit: add comma after "request"

Done.

https://codereview.chromium.org/886103002/diff/20001/ui/display/chromeos/disp...
File ui/display/chromeos/display_configurator_unittest.cc (right):

https://codereview.chromium.org/886103002/diff/20001/ui/display/chromeos/disp...
ui/display/chromeos/display_configurator_unittest.cc:453:
CheckAndResetCallback(true);
On 2015/01/30 17:14:07, Daniel Erat wrote:
> CheckAndResetCallback() failures won't produce meaningful line numbers or
stack
> traces, will they? how about using an enum instead?
> 
>   enum CallbackResult {
>     CALLBACK_SUCCESS,
>     CALLBACK_FAILURE,
>     CALLBACK_NOT_CALLED,
>   }
> 
>   CallbackResult PopCallbackResult() {
>     // choose enum value, clear the "called" field, and return
>   }
> 
> and then in the tests:
> 
>   EXPECT_EQ(CALLBACK_SUCCESS, PopCallbackResult());

Done.

[Daniel Erat, 2015-01-30 19:34:18]

lgtm

[dnicoara, 2015-01-30 20:30:37]

The CQ bit was checked by dnicoara@chromium.org

[commit-bot: I haz the power, 2015-01-30 20:32:12]

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

[commit-bot: I haz the power, 2015-01-30 20:40:36]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-01-30 20:42:00]

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