Implement support for monitor suspend.

Implement support for monitor suspend.

This speeds up idle resume by around 2.5 seconds.

Previously, we would turn the panel off before idle suspend. Then we would
suspend and resume. On resume, the kernel would restore the "off" state, and
the Chrome monitor code would kick in to turn the panel on. This is a huge
waste of time when each panel configuration step takes more than one second.

Instead, just before suspending, we turn the backlight off and the panel on.
On resume the kernel sets the panel state to on right away, and power_manager
turns the backlight on. The Chrome monitor code still runs, but is a noop.

This change needs this power_manager change to handle the backlight part:
https://gerrit.chromium.org/gerrit/#/c/40971/

BUG=chrome-os-partner:13364
TEST=By hand: reduce the idle suspend timeout, let the machine idle and
TEST=suspend, press a key to resume and see how long it takes. The time
TEST=it takes shrinks from ~4 seconds to ~1.5 second.

Change-Id: I610995c12ed08624eb0bc91057d7c9bfa8002fbf

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

[Daniel Erat, 2013-01-08 00:51:35]

just some nits

https://codereview.chromium.org/11783020/diff/1/chrome/browser/chromeos/power...
File chrome/browser/chromeos/power/suspend_observer.h (right):

https://codereview.chromium.org/11783020/diff/1/chrome/browser/chromeos/power...
chrome/browser/chromeos/power/suspend_observer.h:15: // A class to observe
suspend events
nit: add a trailing period

https://codereview.chromium.org/11783020/diff/1/chrome/browser/chromeos/power...
chrome/browser/chromeos/power/suspend_observer.h:23: virtual void
SuspendImminent() OVERRIDE;
nit: add a blank line after this one

https://codereview.chromium.org/11783020/diff/1/chromeos/dbus/power_manager_c...
File chromeos/dbus/power_manager_client.cc (right):

https://codereview.chromium.org/11783020/diff/1/chromeos/dbus/power_manager_c...
chromeos/dbus/power_manager_client.cc:37: has_delay_id_(false),
nit: rename to be more descriptive: has_suspend_delay_id_

also initialize suspend_delay_id_ to something like 0 or -1 here (just so it
doesn't contain a random value that could make it hard to investigate bugs
later)

https://codereview.chromium.org/11783020/diff/1/chromeos/dbus/power_manager_c...
chromeos/dbus/power_manager_client.cc:115:
power_manager::kRegisterSuspendDelayMethod);
nit: fix indenting

https://codereview.chromium.org/11783020/diff/1/chromeos/dbus/power_manager_c...
chromeos/dbus/power_manager_client.cc:121: protobuf_request.Clear();
nit: don't need to call Clear() on a new protobuf, i don't think

https://codereview.chromium.org/11783020/diff/1/chromeos/dbus/power_manager_c...
chromeos/dbus/power_manager_client.cc:125: LOG(ERROR) << "Error calling "
nit: change this to "Error constructing message for " <<
power_manager::kRegisterSuspendDelayMethod;

https://codereview.chromium.org/11783020/diff/1/chromeos/dbus/power_manager_c...
chromeos/dbus/power_manager_client.cc:141: return;
i'm pretty sure you don't want to bail out early from the d'tor like this --
what if someone else adds something unrelated to it later?  i'd do:

  if (has_delay_id_) {
    dbus::MethodCall ...
  }

instead.  if you want to log an error if registration fails, it'd be better to
do it in response to the RegisterSuspendDelay call failing.

https://codereview.chromium.org/11783020/diff/1/chromeos/dbus/power_manager_c...
chromeos/dbus/power_manager_client.cc:503: dbus::Response* response) {
nit: unwrap this line

https://codereview.chromium.org/11783020/diff/1/chromeos/dbus/power_manager_c...
chromeos/dbus/power_manager_client.cc:514: << response->ToString();
nit: change this to be more descriptive, like:

  "Unable to parse reply from " << power_manager::kRegisterSuspendDelayMethod

i don't think that logging the response here is going to be useful; it'll just
be a bunch of binary data

https://codereview.chromium.org/11783020/diff/1/chromeos/dbus/power_manager_c...
chromeos/dbus/power_manager_client.cc:578:
power_manager::kHandleSuspendReadinessMethod);
nit: fix indenting

https://codereview.chromium.org/11783020/diff/1/chromeos/dbus/power_manager_c...
chromeos/dbus/power_manager_client.cc:582: protobuf_request.Clear();
nit: don't need Clear() here

https://codereview.chromium.org/11783020/diff/1/chromeos/dbus/power_manager_c...
chromeos/dbus/power_manager_client.cc:587: LOG(ERROR) << "Error calling "
nit: "Error constructing message for " << ...

https://codereview.chromium.org/11783020/diff/1/chromeos/dbus/power_manager_c...
chromeos/dbus/power_manager_client.cc:659: int32 delay_id_;
nit: rename this to suspend_delay_id_, too

https://codereview.chromium.org/11783020/diff/1/chromeos/dbus/power_manager_c...
File chromeos/dbus/power_manager_client.h (right):

https://codereview.chromium.org/11783020/diff/1/chromeos/dbus/power_manager_c...
chromeos/dbus/power_manager_client.h:29: typedef base::Callback<void(void)>
SuspendImminentCallback;
nit: you don't need this, do you?

https://codereview.chromium.org/11783020/diff/1/chromeos/dbus/power_manager_c...
chromeos/dbus/power_manager_client.h:71: // Called when the system suspends.
nit: i'd add a bit more details about this:

// Called when the system is about to suspend.  Suspend is
// deferred until all observers' implementations of this
// method have finished running.

https://codereview.chromium.org/11783020/diff/1/chromeos/display/output_confi...
File chromeos/display/output_configurator.cc (right):

https://codereview.chromium.org/11783020/diff/1/chromeos/display/output_confi...
chromeos/display/output_configurator.cc:774: // hundreds milliseconds of delay
for the notification, but it would be
nit: s/hundreds/hundred/ or /hundreds of/

https://codereview.chromium.org/11783020/diff/1/chromeos/display/output_confi...
chromeos/display/output_configurator.cc:806: Display* display =
base::MessagePumpAuraX11::GetDefaultXDisplay();
nit: remove this line and just move this down to the place where you use it:

  XSync(base::MessagePumpAuraX11::GetDefaultXDisplay(), 0);

https://codereview.chromium.org/11783020/diff/1/chromeos/display/output_confi...
File chromeos/display/output_configurator.h (right):

https://codereview.chromium.org/11783020/diff/1/chromeos/display/output_confi...
chromeos/display/output_configurator.h:104: void SuspendDisplays();
nit: add a blank line after this and a comment before it

[marcheu, 2013-01-10 01:03:49]

https://codereview.chromium.org/11783020/diff/1/chrome/browser/chromeos/power...
File chrome/browser/chromeos/power/suspend_observer.h (right):

https://codereview.chromium.org/11783020/diff/1/chrome/browser/chromeos/power...
chrome/browser/chromeos/power/suspend_observer.h:15: // A class to observe
suspend events
On 2013/01/08 00:51:35, Daniel Erat wrote:
> nit: add a trailing period

Done.

https://codereview.chromium.org/11783020/diff/1/chrome/browser/chromeos/power...
chrome/browser/chromeos/power/suspend_observer.h:23: virtual void
SuspendImminent() OVERRIDE;
On 2013/01/08 00:51:35, Daniel Erat wrote:
> nit: add a blank line after this one

Done.

https://codereview.chromium.org/11783020/diff/1/chromeos/dbus/power_manager_c...
File chromeos/dbus/power_manager_client.cc (right):

https://codereview.chromium.org/11783020/diff/1/chromeos/dbus/power_manager_c...
chromeos/dbus/power_manager_client.cc:37: has_delay_id_(false),
On 2013/01/08 00:51:35, Daniel Erat wrote:
> nit: rename to be more descriptive: has_suspend_delay_id_
> 
> also initialize suspend_delay_id_ to something like 0 or -1 here (just so it
> doesn't contain a random value that could make it hard to investigate bugs
> later)

Done.

https://codereview.chromium.org/11783020/diff/1/chromeos/dbus/power_manager_c...
chromeos/dbus/power_manager_client.cc:115:
power_manager::kRegisterSuspendDelayMethod);
On 2013/01/08 00:51:35, Daniel Erat wrote:
> nit: fix indenting

Done.

https://codereview.chromium.org/11783020/diff/1/chromeos/dbus/power_manager_c...
chromeos/dbus/power_manager_client.cc:121: protobuf_request.Clear();
On 2013/01/08 00:51:35, Daniel Erat wrote:
> nit: don't need to call Clear() on a new protobuf, i don't think

Done.

https://codereview.chromium.org/11783020/diff/1/chromeos/dbus/power_manager_c...
chromeos/dbus/power_manager_client.cc:125: LOG(ERROR) << "Error calling "
On 2013/01/08 00:51:35, Daniel Erat wrote:
> nit: change this to "Error constructing message for " <<
> power_manager::kRegisterSuspendDelayMethod;

Done.

https://codereview.chromium.org/11783020/diff/1/chromeos/dbus/power_manager_c...
chromeos/dbus/power_manager_client.cc:141: return;
On 2013/01/08 00:51:35, Daniel Erat wrote:
> i'm pretty sure you don't want to bail out early from the d'tor like this --
> what if someone else adds something unrelated to it later?  i'd do:
> 
>   if (has_delay_id_) {
>     dbus::MethodCall ...
>   }
> 
> instead.  if you want to log an error if registration fails, it'd be better to
> do it in response to the RegisterSuspendDelay call failing.

Done.

https://codereview.chromium.org/11783020/diff/1/chromeos/dbus/power_manager_c...
chromeos/dbus/power_manager_client.cc:503: dbus::Response* response) {
On 2013/01/08 00:51:35, Daniel Erat wrote:
> nit: unwrap this line

Done.

https://codereview.chromium.org/11783020/diff/1/chromeos/dbus/power_manager_c...
chromeos/dbus/power_manager_client.cc:514: << response->ToString();
On 2013/01/08 00:51:35, Daniel Erat wrote:
> nit: change this to be more descriptive, like:
> 
>   "Unable to parse reply from " << power_manager::kRegisterSuspendDelayMethod
> 
> i don't think that logging the response here is going to be useful; it'll just
> be a bunch of binary data

Done.

https://codereview.chromium.org/11783020/diff/1/chromeos/dbus/power_manager_c...
chromeos/dbus/power_manager_client.cc:578:
power_manager::kHandleSuspendReadinessMethod);
On 2013/01/08 00:51:35, Daniel Erat wrote:
> nit: fix indenting

Done.

https://codereview.chromium.org/11783020/diff/1/chromeos/dbus/power_manager_c...
chromeos/dbus/power_manager_client.cc:582: protobuf_request.Clear();
On 2013/01/08 00:51:35, Daniel Erat wrote:
> nit: don't need Clear() here

Done.

https://codereview.chromium.org/11783020/diff/1/chromeos/dbus/power_manager_c...
chromeos/dbus/power_manager_client.cc:587: LOG(ERROR) << "Error calling "
On 2013/01/08 00:51:35, Daniel Erat wrote:
> nit: "Error constructing message for " << ...

Done.

https://codereview.chromium.org/11783020/diff/1/chromeos/dbus/power_manager_c...
chromeos/dbus/power_manager_client.cc:659: int32 delay_id_;
On 2013/01/08 00:51:35, Daniel Erat wrote:
> nit: rename this to suspend_delay_id_, too

Done.

https://codereview.chromium.org/11783020/diff/1/chromeos/dbus/power_manager_c...
File chromeos/dbus/power_manager_client.h (right):

https://codereview.chromium.org/11783020/diff/1/chromeos/dbus/power_manager_c...
chromeos/dbus/power_manager_client.h:29: typedef base::Callback<void(void)>
SuspendImminentCallback;
On 2013/01/08 00:51:35, Daniel Erat wrote:
> nit: you don't need this, do you?

Removed.

https://codereview.chromium.org/11783020/diff/1/chromeos/dbus/power_manager_c...
chromeos/dbus/power_manager_client.h:71: // Called when the system suspends.
On 2013/01/08 00:51:35, Daniel Erat wrote:
> nit: i'd add a bit more details about this:
> 
> // Called when the system is about to suspend.  Suspend is
> // deferred until all observers' implementations of this
> // method have finished running.

Done.

https://codereview.chromium.org/11783020/diff/1/chromeos/display/output_confi...
File chromeos/display/output_configurator.cc (right):

https://codereview.chromium.org/11783020/diff/1/chromeos/display/output_confi...
chromeos/display/output_configurator.cc:774: // hundreds milliseconds of delay
for the notification, but it would be
On 2013/01/08 00:51:35, Daniel Erat wrote:
> nit: s/hundreds/hundred/ or /hundreds of/

Done.

https://codereview.chromium.org/11783020/diff/1/chromeos/display/output_confi...
chromeos/display/output_configurator.cc:806: Display* display =
base::MessagePumpAuraX11::GetDefaultXDisplay();
On 2013/01/08 00:51:35, Daniel Erat wrote:
> nit: remove this line and just move this down to the place where you use it:
> 
>   XSync(base::MessagePumpAuraX11::GetDefaultXDisplay(), 0);

Done.

https://codereview.chromium.org/11783020/diff/1/chromeos/display/output_confi...
File chromeos/display/output_configurator.h (right):

https://codereview.chromium.org/11783020/diff/1/chromeos/display/output_confi...
chromeos/display/output_configurator.h:104: void SuspendDisplays();
On 2013/01/08 00:51:35, Daniel Erat wrote:
> nit: add a blank line after this and a comment before it

Done.

[Daniel Erat, 2013-01-10 01:47:59]

lgtm

https://codereview.chromium.org/11783020/diff/7001/chromeos/display/output_co...
File chromeos/display/output_configurator.cc (right):

https://codereview.chromium.org/11783020/diff/7001/chromeos/display/output_co...
chromeos/display/output_configurator.cc:139: << ", mode " << config->mode
nit: change this back to VLOG(1) (which will also fix indenting)

[commit-bot: I haz the power, 2013-01-17 01:07:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/marcheu@chromium.org/11783020/15001

[commit-bot: I haz the power, 2013-01-17 01:07:43]

Presubmit check for 11783020-15001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

Presubmit checks took 1.2s to calculate.

[marcheu, 2013-01-17 01:11:29]

sky, can you please look ack the gypi change?

[sky, 2013-01-17 01:54:38]

LGTM

[commit-bot: I haz the power, 2013-01-17 02:16:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/marcheu@chromium.org/11783020/15001

[commit-bot: I haz the power, 2013-01-17 03:07:50]

Retried try job too often on linux_chromeos for step(s) aura_unittests,
base_unittests, browser_tests, cacheinvalidation_unittests, check_deps,
chromeos_unittests, content_browsertests, content_unittests, crypto_unittests,
dbus_unittests, device_unittests, gpu_unittests, interactive_ui_tests,
ipc_tests, jingle_unittests, media_unittests, net_unittests, ppapi_unittests,
printing_unittests, sandbox_linux_unittests, sql_unittests, sync_unit_tests,
unit_tests

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

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/marcheu@chromium.org/11783020/36001

[commit-bot: I haz the power, 2013-01-23 01:47:11]

Change committed as 178195

[Daniel Erat, 2013-01-24 22:17:38]

LGTM

Thanks!

https://codereview.chromium.org/11783020/diff/45002/chromeos/dbus/power_manag...
File chromeos/dbus/power_manager_client.cc (right):

https://codereview.chromium.org/11783020/diff/45002/chromeos/dbus/power_manag...
chromeos/dbus/power_manager_client.cc:140: // - The lifetime of the
PowerManagerClientImpl can extent past that of
nit: s/extent/extend/

https://codereview.chromium.org/11783020/diff/45002/chromeos/dbus/power_manag...
chromeos/dbus/power_manager_client.cc:143: //   unregister our suspend
notifications.
nit: s/notifications/delay/