power save blocker: Don't try to block power saving in headless mode

power save blocker: Don't try to block power saving in headless mode

When running headless mode, there's no need to block power saving as
there is no available display server. This patch makes PowerSaveBlocker
early-out in such a case (on X11).

BUG=678948
Review-Url: https://codereview.chromium.org/2631153003
Cr-Commit-Position: refs/heads/master@{#444118}
Committed: https://chromium.googlesource.com/chromium/src/+/28b1bf2e4e780f510109ca4b1d61e72f024232aa

[Sami, 2017-01-16 11:28:21]

Description was changed from

==========
power save blocker: Don't try to block power saving in headless mode

When running headless mode, there's no need to block power saving as
there is no available display server. This patch makes PowerSaveBlocker
early-out in such a case (on X11).

BUG=678948
==========

to

==========
power save blocker: Don't try to block power saving in headless mode

When running headless mode, there's no need to block power saving as
there is no available display server. This patch makes PowerSaveBlocker
early-out in such a case (on X11).

BUG=678948
==========

[Sami, 2017-01-16 11:28:22]

skyostil@chromium.org changed reviewers:
+ hashimoto@chromium.org

[Sami, 2017-01-16 11:29:47]

skyostil@chromium.org changed reviewers:
+ msw@chromium.org

[Sami, 2017-01-16 11:29:48]

+hashimoto@ for OWNERs review and +msw@ for the new dep on ui/gfx. PTAL, thanks!

[Sami, 2017-01-16 11:29:55]

The CQ bit was checked by skyostil@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-16 11:30:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-16 12:30:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-16 12:30:01]

Dry run: This issue passed the CQ dry run.

[hashimoto, 2017-01-17 02:08:13]

https://codereview.chromium.org/2631153003/diff/1/device/power_save_blocker/DEPS
File device/power_save_blocker/DEPS (right):

https://codereview.chromium.org/2631153003/diff/1/device/power_save_blocker/D...
device/power_save_blocker/DEPS:5: "+ui/gfx",
ui/gfx/x is unneeded when you have ui/gfx.

https://codereview.chromium.org/2631153003/diff/1/device/power_save_blocker/p...
File device/power_save_blocker/power_save_blocker_x11.cc (right):

https://codereview.chromium.org/2631153003/diff/1/device/power_save_blocker/p...
device/power_save_blocker/power_save_blocker_x11.cc:490: if
(base::CommandLine::ForCurrentProcess()->HasSwitch(switches::kHeadless))
1. Instead of adding if to the ctor and the dtor, please modify
PowerSaveBlocker::Delegate::SelectAPI() above to return NO_API
2. I'm not familiar with base::nix::GetDesktopEnvironment(), but shouldn't it
detect it when X11 is not available?
3. Do we really want to always disable poser save blocker when running headless?

[Sami, 2017-01-17 13:44:29]

Thanks!

https://codereview.chromium.org/2631153003/diff/1/device/power_save_blocker/DEPS
File device/power_save_blocker/DEPS (right):

https://codereview.chromium.org/2631153003/diff/1/device/power_save_blocker/D...
device/power_save_blocker/DEPS:5: "+ui/gfx",
On 2017/01/17 02:08:12, hashimoto wrote:
> ui/gfx/x is unneeded when you have ui/gfx.

Ah, true, fixed.

https://codereview.chromium.org/2631153003/diff/1/device/power_save_blocker/p...
File device/power_save_blocker/power_save_blocker_x11.cc (right):

https://codereview.chromium.org/2631153003/diff/1/device/power_save_blocker/p...
device/power_save_blocker/power_save_blocker_x11.cc:490: if
(base::CommandLine::ForCurrentProcess()->HasSwitch(switches::kHeadless))
On 2017/01/17 02:08:12, hashimoto wrote:
> 1. Instead of adding if to the ctor and the dtor, please modify
> PowerSaveBlocker::Delegate::SelectAPI() above to return NO_API

Good idea, done. (Note that I had to do the same to the X screensaver related
code.)

> 2. I'm not familiar with base::nix::GetDesktopEnvironment(), but shouldn't it
> detect it when X11 is not available?

Looks like it doesn't check for X11 as such, which I think is correct since not
all the detected environments actually require X11.

> 3. Do we really want to always disable poser save blocker when running
headless?

I believe so. There's no API we could use to control power saving so the blocker
can't really do anything in the first place.

[Sami, 2017-01-17 14:13:40]

The CQ bit was checked by skyostil@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-17 14:13:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-17 15:05:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-17 15:05:05]

Dry run: This issue passed the CQ dry run.

[hashimoto, 2017-01-17 15:28:33]

Thanks,

lgtm with requests.

https://codereview.chromium.org/2631153003/diff/20001/device/power_save_block...
File device/power_save_blocker/power_save_blocker_x11.cc (right):

https://codereview.chromium.org/2631153003/diff/20001/device/power_save_block...
device/power_save_blocker/power_save_blocker_x11.cc:440: if
(base::CommandLine::ForCurrentProcess()->HasSwitch(switches::kHeadless))
Please add a brief comment to justify this behavior.

https://codereview.chromium.org/2631153003/diff/20001/device/power_save_block...
device/power_save_blocker/power_save_blocker_x11.cc:458: if
(base::CommandLine::ForCurrentProcess()->HasSwitch(switches::kHeadless))
ditto.

[Sami, 2017-01-17 15:43:49]

Thank you!

https://codereview.chromium.org/2631153003/diff/20001/device/power_save_block...
File device/power_save_blocker/power_save_blocker_x11.cc (right):

https://codereview.chromium.org/2631153003/diff/20001/device/power_save_block...
device/power_save_blocker/power_save_blocker_x11.cc:440: if
(base::CommandLine::ForCurrentProcess()->HasSwitch(switches::kHeadless))
On 2017/01/17 15:28:32, hashimoto wrote:
> Please add a brief comment to justify this behavior.

Done.

https://codereview.chromium.org/2631153003/diff/20001/device/power_save_block...
device/power_save_blocker/power_save_blocker_x11.cc:458: if
(base::CommandLine::ForCurrentProcess()->HasSwitch(switches::kHeadless))
On 2017/01/17 15:28:32, hashimoto wrote:
> ditto.

Done.

[hashimoto, 2017-01-17 16:18:26]

lgtm++

[msw, 2017-01-17 18:54:31]

expanded ui/gfx dependency lgtm

[Sami, 2017-01-17 18:56:41]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2017-01-17 18:57:21]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-17 20:04:16]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1484679401590970,
"parent_rev": "ef71a0a844aed50e40cf74d5c5f1ac0b94639c18", "commit_rev":
"28b1bf2e4e780f510109ca4b1d61e72f024232aa"}

[commit-bot: I haz the power, 2017-01-17 20:04:53]

Description was changed from

==========
power save blocker: Don't try to block power saving in headless mode

When running headless mode, there's no need to block power saving as
there is no available display server. This patch makes PowerSaveBlocker
early-out in such a case (on X11).

BUG=678948
==========

to

==========
power save blocker: Don't try to block power saving in headless mode

When running headless mode, there's no need to block power saving as
there is no available display server. This patch makes PowerSaveBlocker
early-out in such a case (on X11).

BUG=678948

Review-Url: https://codereview.chromium.org/2631153003
Cr-Commit-Position: refs/heads/master@{#444118}
Committed:
https://chromium.googlesource.com/chromium/src/+/28b1bf2e4e780f510109ca4b1d61...
==========

[commit-bot: I haz the power, 2017-01-17 20:04:54]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/28b1bf2e4e780f510109ca4b1d61...