Add lucid sleep flag and debugging helper

Add lucid sleep flag and debugging helper

Add a flag for lucid sleep so that apps that wish to start working with it can
do so while it's still under development.  Also to help with testing when the
lid is closed, add a class that will flash the light bar when the system is in dark
resume.

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

Committed: https://crrev.com/6e3f35be628105d6d5d2c570de866f22bcd1f2bc
Cr-Commit-Position: refs/heads/master@{#294972}

[Chirantan Ekbote, 2014-09-12 18:47:06]

chirantan@chromium.org changed reviewers:
+ asvitkine@chromium.org, derat@chromium.org

[Chirantan Ekbote, 2014-09-12 18:47:06]

We plan on landing lucid sleep functionality behind a flag in M39.  Dan, please
review for chromeos.  Alexei, please review for histograms.xml.

[Daniel Erat, 2014-09-12 19:53:03]

https://codereview.chromium.org/566993002/diff/1/chrome/app/chromeos_strings....
File chrome/app/chromeos_strings.grdp (right):

https://codereview.chromium.org/566993002/diff/1/chrome/app/chromeos_strings....
chrome/app/chromeos_strings.grdp:1094: Enable Lucid Sleep.
have you been talking to anyone on the UX side about this? i know it's just
chrome://flags, but i'm wondering if "lucid sleep" is the name we want to use
when exposing this publicly or if we want something that's less cryptic. "lucid
sleep" has always sounded weird to me; "lucid dreaming" is the actual term
that's commonly used.

kuscher@ is probably the first person i'd ask about this, but he doesn't appear
to be online right now. i asked kanliu@ and he said he's going to ask around.

https://codereview.chromium.org/566993002/diff/1/chrome/browser/chromeos/chro...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/566993002/diff/1/chrome/browser/chromeos/chro...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:566: lightbar_.reset(new
LightBar());
can you instead check the lucid sleep flag here and only create the object if
it's set? that pulls some complexity out of the LightBar class

https://codereview.chromium.org/566993002/diff/1/chrome/browser/chromeos/chro...
File chrome/browser/chromeos/chrome_browser_main_chromeos.h (right):

https://codereview.chromium.org/566993002/diff/1/chrome/browser/chromeos/chro...
chrome/browser/chromeos/chrome_browser_main_chromeos.h:77: scoped_ptr<LightBar>
lightbar_;
nit: should maybe be light_bar_ to match the class name

https://codereview.chromium.org/566993002/diff/1/chrome/browser/chromeos/powe...
File chrome/browser/chromeos/power/lightbar.cc (right):

https://codereview.chromium.org/566993002/diff/1/chrome/browser/chromeos/powe...
chrome/browser/chromeos/power/lightbar.cc:5: #include
"chrome/browser/chromeos/power/lightbar.h"
nit: this should also be light_bar.h unless the class is renamed to Lightbar

https://codereview.chromium.org/566993002/diff/1/chrome/browser/chromeos/powe...
chrome/browser/chromeos/power/lightbar.cc:19: const int64_t
kDarkSuspendDelaySeconds = 4;
nit: int is fine if you don't specifically need 64 bits

https://codereview.chromium.org/566993002/diff/1/chrome/browser/chromeos/powe...
chrome/browser/chromeos/power/lightbar.cc:26: void
LightBar::DarkSuspendImminent() {
nit: move this down to match the order in the .h file

https://codereview.chromium.org/566993002/diff/1/chrome/browser/chromeos/powe...
chrome/browser/chromeos/power/lightbar.cc:44: if (base::WriteFile(control_path_,
kKonamiCommand, strlen(kKonamiCommand)) !=
is there a plan to make a distinction between the dark resumes that are
initiated by powerd to possibly shut down if the battery level is low and ones
that happen in response to incoming messages? it seems weird to flash the light
bar in the former case; those are explicitly supposed to not be visible to the
user

https://codereview.chromium.org/566993002/diff/1/chrome/browser/chromeos/powe...
File chrome/browser/chromeos/power/lightbar.h (right):

https://codereview.chromium.org/566993002/diff/1/chrome/browser/chromeos/powe...
chrome/browser/chromeos/power/lightbar.h:21: // PowerManagerClient::Observer
implementation
nit: add trailing period or colon

[Alexei Svitkine (slow), 2014-09-12 20:30:28]

histograms.xml lgtm, I didn't review the rest

[Daniel Erat, 2014-09-12 21:18:58]

https://codereview.chromium.org/566993002/diff/1/chrome/app/chromeos_strings....
File chrome/app/chromeos_strings.grdp (right):

https://codereview.chromium.org/566993002/diff/1/chrome/app/chromeos_strings....
chrome/app/chromeos_strings.grdp:1094: Enable Lucid Sleep.
On 2014/09/12 19:53:03, Daniel Erat wrote:
> have you been talking to anyone on the UX side about this? i know it's just
> chrome://flags, but i'm wondering if "lucid sleep" is the name we want to use
> when exposing this publicly or if we want something that's less cryptic.
"lucid
> sleep" has always sounded weird to me; "lucid dreaming" is the actual term
> that's commonly used.
> 
> kuscher@ is probably the first person i'd ask about this, but he doesn't
appear
> to be online right now. i asked kanliu@ and he said he's going to ask around.

per kan: "alexandra will come up with something else to describe it without
using a name like lucid sleep"

[Chirantan Ekbote, 2014-09-15 23:20:46]

ptal

https://codereview.chromium.org/566993002/diff/1/chrome/app/chromeos_strings....
File chrome/app/chromeos_strings.grdp (right):

https://codereview.chromium.org/566993002/diff/1/chrome/app/chromeos_strings....
chrome/app/chromeos_strings.grdp:1094: Enable Lucid Sleep.
On 2014/09/12 at 21:18:57, Daniel Erat wrote:
> On 2014/09/12 19:53:03, Daniel Erat wrote:
> > have you been talking to anyone on the UX side about this? i know it's just
> > chrome://flags, but i'm wondering if "lucid sleep" is the name we want to
use
> > when exposing this publicly or if we want something that's less cryptic.
"lucid
> > sleep" has always sounded weird to me; "lucid dreaming" is the actual term
> > that's commonly used.
> > 
> > kuscher@ is probably the first person i'd ask about this, but he doesn't
appear
> > to be online right now. i asked kanliu@ and he said he's going to ask
around.
> 
> per kan: "alexandra will come up with something else to describe it without
using a name like lucid sleep"

updated with new strings from Alex.

https://codereview.chromium.org/566993002/diff/1/chrome/browser/chromeos/powe...
File chrome/browser/chromeos/power/lightbar.cc (right):

https://codereview.chromium.org/566993002/diff/1/chrome/browser/chromeos/powe...
chrome/browser/chromeos/power/lightbar.cc:44: if (base::WriteFile(control_path_,
kKonamiCommand, strlen(kKonamiCommand)) !=
On 2014/09/12 at 19:53:03, Daniel Erat wrote:
> is there a plan to make a distinction between the dark resumes that are
initiated by powerd to possibly shut down if the battery level is low and ones
that happen in response to incoming messages? it seems weird to flash the light
bar in the former case; those are explicitly supposed to not be visible to the
user

Yes, Derek is working on being able to distinguish network wakes from rtc wakes.
 Eric is also looking into separating rtc wakes for shutdown-after-x from rtc
wakes for dhcp renewals and gcm heartbeat messages (crosbug.com/p/31970).

Also, the light bar code is only going to live for one release.  I plan on
removing it once we're ready to ship.

[Daniel Erat, 2014-09-15 23:24:36]

https://codereview.chromium.org/566993002/diff/20001/chrome/app/chromeos_stri...
File chrome/app/chromeos_strings.grdp (right):

https://codereview.chromium.org/566993002/diff/20001/chrome/app/chromeos_stri...
chrome/app/chromeos_strings.grdp:1094: Wake On Packets.
nit: this string shouldn't have a trailing period (the other flag names don't)

https://codereview.chromium.org/566993002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/power/light_bar.cc (right):

https://codereview.chromium.org/566993002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/power/light_bar.cc:44:
base::MessageLoop::current()->PostDelayedTask(
move this to the end and only do it if the write was successful?

[Chirantan Ekbote, 2014-09-15 23:36:17]

https://codereview.chromium.org/566993002/diff/20001/chrome/app/chromeos_stri...
File chrome/app/chromeos_strings.grdp (right):

https://codereview.chromium.org/566993002/diff/20001/chrome/app/chromeos_stri...
chrome/app/chromeos_strings.grdp:1094: Wake On Packets.
On 2014/09/15 at 23:24:36, Daniel Erat wrote:
> nit: this string shouldn't have a trailing period (the other flag names don't)

done

https://codereview.chromium.org/566993002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/power/light_bar.cc (right):

https://codereview.chromium.org/566993002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/power/light_bar.cc:44:
base::MessageLoop::current()->PostDelayedTask(
On 2014/09/15 at 23:24:36, Daniel Erat wrote:
> move this to the end and only do it if the write was successful?

The light bar and the delay are pretty much independent.  We always want to do
the delay because there is currently no way to actually track the progress of
events through chrome and this is a hacky workaround.  Flashing the light bar is
best effort on devices that have one and is mainly a debugging aid rather than
something necessary for the feature to work.

[Daniel Erat, 2014-09-15 23:37:16]

lgtm

[Chirantan Ekbote, 2014-09-15 23:38:22]

The CQ bit was checked by chirantan@chromium.org

[commit-bot: I haz the power, 2014-09-16 00:01:14]

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

[commit-bot: I haz the power, 2014-09-16 02:07:34]

Committed patchset #3 (id:40001) as 8d9f62dee7306fa004114a94bdb7fe8c3f9a0a12

[commit-bot: I haz the power, 2014-09-16 02:11:44]

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

[Lei Zhang, 2014-09-19 21:08:47]

https://codereview.chromium.org/566993002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/power/light_bar.cc (right):

https://codereview.chromium.org/566993002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/power/light_bar.cc:10: #include "base/file_util.h"
Please be aware that this is going away. Use base/files/file_util.h in the
future. I had to removed all references, but they keep sneaking back in.