Add dark resume methods to PowerManagerClient

This makes PowerManagerClient tell the power manager to notify chrome
when the system comes up in dark resume.  It also adds a
DarkSuspendImminent() method that observers within chrome can implement
if they want to be notified of this event.

BUG=397346
Signed-off-by: Chirantan Ekbote <chirantan@chromium.org>;

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=290692

[Chirantan Ekbote, 2014-08-15 23:04:16]

Please take a look.

[Daniel Erat, 2014-08-15 23:49:31]

https://codereview.chromium.org/438433002/diff/40001/chromeos/dbus/power_mana...
File chromeos/dbus/power_manager_client.cc (right):

https://codereview.chromium.org/438433002/diff/40001/chromeos/dbus/power_mana...
chromeos/dbus/power_manager_client.cc:468: void
DarkSuspendImminentReceived(dbus::Signal* signal) {
can you just eliminate these and bind true or false to the shared method and use
that as the callback?

https://codereview.chromium.org/438433002/diff/40001/chromeos/dbus/power_mana...
chromeos/dbus/power_manager_client.cc:472: void SuspendImminentCommon(bool
in_dark_resume,
nit: HandleSuspendImminent()?

https://codereview.chromium.org/438433002/diff/40001/chromeos/dbus/power_mana...
chromeos/dbus/power_manager_client.cc:499: << "waiting on attempt " <<
pending_suspend_id_;
i'm trying to think of when this could happen -- when someone within chrome
registers an async delay and then waits too long to call it, resulting in the
delay timing out and powerd suspending anyway?

in general, does suspend_is_pending_ being true here indicate a problem?

https://codereview.chromium.org/438433002/diff/40001/chromeos/dbus/power_mana...
chromeos/dbus/power_manager_client.cc:506: if (suspending_from_dark_resume_) {
nit: omit curly brackets

https://codereview.chromium.org/438433002/diff/40001/chromeos/dbus/power_mana...
chromeos/dbus/power_manager_client.cc:636: power_manager_proxy_->CallMethod(
add a helper method for this to cut down on the duplicated code?

https://codereview.chromium.org/438433002/diff/40001/chromeos/dbus/power_mana...
chromeos/dbus/power_manager_client.cc:714: bool suspending_from_dark_resume_;
add a comment clarifying what this means, and that it affects the interpretation
of pending_suspend_id_ and suspend_is_pending_

https://codereview.chromium.org/438433002/diff/40001/chromeos/dbus/power_mana...
File chromeos/dbus/power_manager_client.h (right):

https://codereview.chromium.org/438433002/diff/40001/chromeos/dbus/power_mana...
chromeos/dbus/power_manager_client.h:76: // finished running and those observers
that wish to asynchromously delay
nit: s/asynchromously/asynchronously/

https://codereview.chromium.org/438433002/diff/40001/chromeos/dbus/power_mana...
chromeos/dbus/power_manager_client.h:77: // the suspend should call
PowerManagerClient::GetSuspendReadinessCallback()
sharing GetSuspendReadinessCallback() for this probably isn't the right
approach; i'm concerned that some observers that are just interested in normal
suspends will fail to call this before dark suspends. adding a new
GetDarkSuspendReadinessCallback() method is probably the best approach.

[Chirantan Ekbote, 2014-08-18 17:49:08]

https://codereview.chromium.org/438433002/diff/40001/chromeos/dbus/power_mana...
File chromeos/dbus/power_manager_client.cc (right):

https://codereview.chromium.org/438433002/diff/40001/chromeos/dbus/power_mana...
chromeos/dbus/power_manager_client.cc:468: void
DarkSuspendImminentReceived(dbus::Signal* signal) {
sure.  will fix.

https://codereview.chromium.org/438433002/diff/40001/chromeos/dbus/power_mana...
chromeos/dbus/power_manager_client.cc:499: << "waiting on attempt " <<
pending_suspend_id_;
Yes, that's the situation in which this could happen.  I don't _think_ that it's
a big problem if suspend_is_pending_ is true here but I guess it depends on what
the delays are doing.  From the PowerManagerClient's perspective, we would let
any outstanding delays continue running and when they run the suspend readiness
callback we would just ignore it.

https://codereview.chromium.org/438433002/diff/40001/chromeos/dbus/power_mana...
File chromeos/dbus/power_manager_client.h (right):

https://codereview.chromium.org/438433002/diff/40001/chromeos/dbus/power_mana...
chromeos/dbus/power_manager_client.h:77: // the suspend should call
PowerManagerClient::GetSuspendReadinessCallback()
I have no problem with adding a new GetDarkSuspendReadinessCallback() method but
I'm not sure I understand what you mean here.  Observers that only care about
normal suspends should not find out about dark suspends since they presumably
will not implement the DarkSuspendImminent() method.

[Daniel Erat, 2014-08-18 21:02:13]

https://codereview.chromium.org/438433002/diff/60001/chromeos/dbus/power_mana...
File chromeos/dbus/power_manager_client.cc (right):

https://codereview.chromium.org/438433002/diff/60001/chromeos/dbus/power_mana...
chromeos/dbus/power_manager_client.cc:440:
RegisterSuspendDelayReplyCommon(false, response);
nit: OnRegisterSuspendDelayReply()? putting "Register" at the beginning of the
method name makes it seem like it's actually called to register something.

https://codereview.chromium.org/438433002/diff/60001/chromeos/dbus/power_mana...
chromeos/dbus/power_manager_client.cc:514: if (suspending_from_dark_resume_) {
nit: omit curly brackets

https://codereview.chromium.org/438433002/diff/60001/chromeos/dbus/power_mana...
File chromeos/dbus/power_manager_client.h (right):

https://codereview.chromium.org/438433002/diff/60001/chromeos/dbus/power_mana...
chromeos/dbus/power_manager_client.h:79: // observer is ready for suspend.
i embarrassingly forgot how this interface works and thought that observers
would call GetSuspendReadinessCallback() once when they registered themselves
instead of in the middle of a *SuspendImminent() call.

this still seems problematic to me, though -- isn't there now a hidden
dependency that the two ID sequences never overlap lest one late asynchronous
callback accidentally ack a different suspend attempt than the one it was
originally invoked for?

one simple, albeit maybe hacky, way to avoid this would be to also bind a
parameter to the callbacks specifying whether they were generated in response to
regular suspend attempts or dark suspend attempts and then check that it lines
up with the current attempt when the callback is invoked.

[Chirantan Ekbote, 2014-08-18 21:15:43]

https://codereview.chromium.org/438433002/diff/60001/chromeos/dbus/power_mana...
File chromeos/dbus/power_manager_client.cc (right):

https://codereview.chromium.org/438433002/diff/60001/chromeos/dbus/power_mana...
chromeos/dbus/power_manager_client.cc:440:
RegisterSuspendDelayReplyCommon(false, response);
On 2014/08/18 at 21:02:13, Daniel Erat wrote:
> nit: OnRegisterSuspendDelayReply()? putting "Register" at the beginning of the
method name makes it seem like it's actually called to register something.

Sorry, I uploaded this patch set on Friday before I had seen your comments on
the previous patch set.  I've changed the name to
HandleRegisterSuspendDelayReply and I'm using base::Bind to bind the boolean
argument in the callback itself, like you suggested.

https://codereview.chromium.org/438433002/diff/60001/chromeos/dbus/power_mana...
File chromeos/dbus/power_manager_client.h (right):

https://codereview.chromium.org/438433002/diff/60001/chromeos/dbus/power_mana...
chromeos/dbus/power_manager_client.h:79: // observer is ready for suspend.
On 2014/08/18 at 21:02:13, Daniel Erat wrote:
> i embarrassingly forgot how this interface works and thought that observers
would call GetSuspendReadinessCallback() once when they registered themselves
instead of in the middle of a *SuspendImminent() call.
> 
> this still seems problematic to me, though -- isn't there now a hidden
dependency that the two ID sequences never overlap lest one late asynchronous
callback accidentally ack a different suspend attempt than the one it was
originally invoked for?
> 

True, but with the way powerd generates IDs we would need 2^15 suspend attempts
and 0 dark suspend attempts before the two IDs would risk overlapping.

> one simple, albeit maybe hacky, way to avoid this would be to also bind a
parameter to the callbacks specifying whether they were generated in response to
regular suspend attempts or dark suspend attempts and then check that it lines
up with the current attempt when the callback is invoked.

sgtm.  We can bind the value of suspending_from_dark_resume_ and then check it
when the callback is run.

[Chirantan Ekbote, 2014-08-19 01:13:30]

Please take another look.

[Daniel Erat, 2014-08-19 02:13:17]

thanks, lgtm with a few small comments

https://codereview.chromium.org/438433002/diff/80001/chromeos/dbus/power_mana...
File chromeos/dbus/power_manager_client.cc (right):

https://codereview.chromium.org/438433002/diff/80001/chromeos/dbus/power_mana...
chromeos/dbus/power_manager_client.cc:654: int32 delay_id;
nit: initialize this to -1 or 0 or something

i think that int32 and friends are deprecated in favor of e.g. int32_t now, too

https://codereview.chromium.org/438433002/diff/80001/chromeos/dbus/power_mana...
chromeos/dbus/power_manager_client.cc:707: // method is called
(SuspendImminent() or DarkSuspendImminent()).
this isn't exactly true, is it? the signal that powerd emits determines which
method is called on the observers; this variable just tracks the state after
that

[Chirantan Ekbote, 2014-08-19 18:18:26]

https://codereview.chromium.org/438433002/diff/80001/chromeos/dbus/power_mana...
File chromeos/dbus/power_manager_client.cc (right):

https://codereview.chromium.org/438433002/diff/80001/chromeos/dbus/power_mana...
chromeos/dbus/power_manager_client.cc:654: int32 delay_id;
On 2014/08/19 at 02:13:17, Daniel Erat wrote:
> nit: initialize this to -1 or 0 or something
> 
> i think that int32 and friends are deprecated in favor of e.g. int32_t now,
too

Done.  There were a few other int32s in this file.  I've changed those as well.

https://codereview.chromium.org/438433002/diff/80001/chromeos/dbus/power_mana...
chromeos/dbus/power_manager_client.cc:707: // method is called
(SuspendImminent() or DarkSuspendImminent()).
On 2014/08/19 at 02:13:17, Daniel Erat wrote:
> this isn't exactly true, is it? the signal that powerd emits determines which
method is called on the observers; this variable just tracks the state after
that

You're right.  I've removed that sentence.

[Chirantan Ekbote, 2014-08-19 22:13:39]

The CQ bit was checked by chirantan@chromium.org

[commit-bot: I haz the power, 2014-08-19 22:14:39]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/chirantan@chromium.org/438433002/120001

[commit-bot: I haz the power, 2014-08-19 23:23:13]

Committed patchset #7 (120001) as 290692

[pprabhu, 2014-08-26 18:13:18]

pprabhu@chromium.org changed reviewers:
+ pprabhu@chromium.org

[pprabhu, 2014-08-26 18:13:18]

@chirantan: I noticed something while looking at this to implement the shill
equivalent. PTAL.

https://codereview.chromium.org/438433002/diff/120001/chromeos/dbus/power_man...
File chromeos/dbus/power_manager_client.cc (right):

https://codereview.chromium.org/438433002/diff/120001/chromeos/dbus/power_man...
chromeos/dbus/power_manager_client.cc:606: void RegisterSuspendDelays() {
Fly by late comment: Shouldn't this also reset |suspend_is_pending_| and
|suspending_from_dark_resume_|?

Consider the case when power_manager dies after we receive a
(Dark)SuspendImminent signal, we will reset the state of PowerManagerClient, but
will end up with incorrect initial values for these two variables.

[Daniel Erat, 2014-08-26 18:16:34]

https://codereview.chromium.org/438433002/diff/120001/chromeos/dbus/power_man...
File chromeos/dbus/power_manager_client.cc (right):

https://codereview.chromium.org/438433002/diff/120001/chromeos/dbus/power_man...
chromeos/dbus/power_manager_client.cc:606: void RegisterSuspendDelays() {
On 2014/08/26 18:13:18, pprabhu wrote:
> Fly by late comment: Shouldn't this also reset |suspend_is_pending_| and
> |suspending_from_dark_resume_|?
> 
> Consider the case when power_manager dies after we receive a
> (Dark)SuspendImminent signal, we will reset the state of PowerManagerClient,
but
> will end up with incorrect initial values for these two variables.

probably wouldn't hurt to do that here, but i think that powerd has an ugly hack
to send a fake SuspendDone event at startup in this case

[Chirantan Ekbote, 2014-08-26 19:10:54]

https://codereview.chromium.org/438433002/diff/120001/chromeos/dbus/power_man...
File chromeos/dbus/power_manager_client.cc (right):

https://codereview.chromium.org/438433002/diff/120001/chromeos/dbus/power_man...
chromeos/dbus/power_manager_client.cc:606: void RegisterSuspendDelays() {
On 2014/08/26 at 18:16:33, Daniel Erat wrote:
> On 2014/08/26 18:13:18, pprabhu wrote:
> > Fly by late comment: Shouldn't this also reset |suspend_is_pending_| and
> > |suspending_from_dark_resume_|?
> > 
> > Consider the case when power_manager dies after we receive a
> > (Dark)SuspendImminent signal, we will reset the state of PowerManagerClient,
but
> > will end up with incorrect initial values for these two variables.
> 
> probably wouldn't hurt to do that here, but i think that powerd has an ugly
hack to send a fake SuspendDone event at startup in this case

But it looks like |suspend_is_pending_| is not cleared in SuspendDoneReceived. 
It only gets cleared in MaybeReportSuspendReadiness.  So we could have a problem
if the following sequence occurs:

- Chrome receives a {Dark}SuspendImminent
- One of the observers takes a callback
- powerd goes away
- powerd restarts
- The observer runs the callback

In this case, chrome would report suspend readiness with an id of -1 or whatever
id powerd assigned to it (depending on whether or not chrome received the
RegisterSuspendDelay reply before running the observer callback).  I think
powerd can deal with either case but we should probably prevent chrome from
sending the message in the first place.  Plus, if for whatever reason powerd
doesn't restart, then chrome would still report suspend readiness only to
receive a ServiceUnknown error in return.

Maybe this should be cleared in NameOwnerChangedReceived?  I'll upload a CL to
do this.