Move Chrome OS power policy code into a device service |
||||||||||
Issue descriptionForking from issue 694076 and pasting some notes about this that I wrote in a doc: chromeos::PowerPolicyController (in //chromeos/dbus) is a singleton that generates power_manager::PowerManagementPolicy protobufs and sends them to powerd over D-Bus using chromeos::PowerManagerClient (also in //chromeos/dbus). PowerPolicyController exposes various public methods that are used to inject the state that it needs to generate the protos: - ApplyPrefs(): Takes a PowerPolicyController::PrefValues struct, which defines timeouts, actions to take, and other aspects of the policy. Called by chromeos::PowerPrefs (in //chrome/browser/chromeos/power), which watches for changes to Chrome's prefs and copies their values to the POD fields in PrefValues. - AddScreenWakeLock(), AddDimWakeLock(), AddSystemWakeLock(): Register new wake locks in an internal map and return IDs that can be used to remove them later. Called by arc::ArcPowerBridge (//components/arc/power) and device::PowerSaveBlocker (//device/power_save_blocker), which is used all over the place (but maybe primarily in Chrome?). - RemoveWakeLock(): Unregisters previously-registered wake locks. - NotifyChromeIsExiting(): Sets an internal bool member that's used to override the lid-closed action so it won't cause the system to suspend while it's in the process of shutting down. Currently called by //chrome/browser/lifetime/application_lifetime.cc, but I'd like to also call it from ash::LockStateController (//ash/wm) for http://crbug.com/694076. Long-term, the policy needs to be constructed from prefs that live in Chrome, wake locks that mostly come from Chrome but may also be needed in ash, and exit notifications coming from ash(?). There's a bigger question about whether the prefs should live in Chrome. They're currently only set by enterprise policies ( issue 633455 tracks adding settings for them), but in a future world where the browser doesn't need to be running, they should still be honored while the user is logged in. --- I've been looking into adding a new (creatively-named :-/) device::mojom::PowerPolicyManager interface that exposes an interface similar in functionality to PowerPolicyController's public interface. The Chrome OS implementation of PowerSaveBlocker would call it to create new locks, the browser would pass prefs to it (at least initially), and so on. I'll probably make PowerPolicyManager call into PowerPolicyController at first, but it'd likely be cleaner to eventually move PPC's implementation directly into PPM later so we don't have a pointless extra layer with all that entails (e.g. mapping between enums). --- Off the top of my head, the following things will be broken in mash until this is done: - Inhibiting screen dim/off or suspend while playing video/audio or via the chrome.power extension API - Enterprise power management policies (e.g. modifying inactivity delays, shutting down on lid-close, etc.) - Avoiding some races if the lid is closed while the system is shutting down
,
Mar 17 2017
Hi Dan, This makes sense to me. One important refinement of the plan that you describe above: We are going to move //device/power_save_blocker to be part of the internal implementation of the Device Service (see bugtree rooted at crbug.com/612346 ). So don't worry about separating //device/power_save_blocker from its //chromeos dependency via Mojo, as the latter code will end up living in the Device Service as well if I'm understanding your description here correctly, and the code dependency will thus be fine in the long term.
,
Mar 17 2017
+jonross re: prefs. It might be possible for ash to "subscribe" to the prefs from the prefs service he built. That service will live in chrome for the time being, so probably it would work with enterprise policy.
,
Mar 17 2017
(What are some example pref names?)
,
Mar 17 2017
+tibell@ FYI From looking at the current chromeos::PowerPrefs there are already several prefs registered in chrome/common/pref_names.h. Such as prefs::kPowerLockScreenDimDelayMs. The prefs service could be used to subscribe to these from outside of the Chrome process. tibell@ is working on pulling the preferences out of Chrome into a standalone service.
,
Mar 17 2017
#2: Thanks, I wasn't aware of the work in //device/wake_lock. Just to make sure I understand the long-term plan: Are all existing PowerSaveBlocker users eventually going to be switched to use the wake lock service? Will the PowerSaveBlocker implementation eventually be moved directly into the service? If so, then yes, it sounds like we'd just want chromeos::PowerPolicyController to live in the same process as the service. I'm not sure if we actually need a new service for power policy in this case. If the same process that hosts PowerPolicyController can subscribe to the prefs mentioned above, then I think that the only remaining piece here is coming up with a way to notify the controller when we're exiting.
,
Mar 17 2017
,
Mar 20 2017
c#6: Yes. Note that the work about prefs is not currently within the scope of the Device Service effort, so perhaps that's what this bug should explicitly describe and track, as well as "coming up with a way to notify the controller when we're exiting." FYI, we're actively working on Wake Lock / PowerSaveBlocker.
,
Oct 21 2017
After a long hiatus, I just looked at this again. Almost all wake lock usage goes through device::mojom::WakeLock now. That uses PowerSaveBlocker, which calls into chromeos::PowerPolicyController. The bad news is that components/arc/power/arc_power_bridge.cc still uses chromeos::PowerPolicyController directly and calls its AddDimWakeLock method, which represents an intermediate level of wake lock that isn't represented in device::mojom::WakeLockType. It's probably straightforward to add a new type and support it on Chrome OS, but we might need to map it to PreventDisplaySleep on other platforms. So, I think the current steps are probably something like this: a) Add a PreventDisplaySleepAllowDimming (any naming suggestions?) value to device::mojom::WakeLockType. b) Make arc::ArcPowerBridge stop using chromeos::PowerPolicyController directly and instead use device::mojom::WakeLock. c) Add a device::mojom::PowerPolicy interface with PrepareForExit and SetEncryptionMigrationActive methods that forward through to PowerPolicyController's NotifyChromeIsExiting and SetEncryptionMigrationActive methods. d) Make PowerPolicyController users in //chrome instead use device::mojom::PowerPolicy. There are some unit test calls to GetPolicyDebugString that may be tricky. Maybe that should be exposed in c) as well? e) Update PowerPolicyController to subscribe to pref updates and remove its ApplyPrefs method (currently called from //chrome). f) Move PowerPolicyController from //chromeos/dbus to somewhere under //services/device. g) Figure out the D-Bus story for the device service.
,
Oct 23 2017
Re: c#9, that all sounds reasonable at a high level, with the caveat that you're much closer to the issues involve here than I am. In particular, all of the work described there and especially (g) would need to be driven by someone from the ChromeOS side, although of course I'd be happy to be involved from a global Device Service perspective.
,
Nov 1 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/256b460474c11b858a12db6e15361eea0f7d3dc7 commit 256b460474c11b858a12db6e15361eea0f7d3dc7 Author: Daniel Erat <derat@chromium.org> Date: Wed Nov 01 21:59:17 2017 service/device: Add wake locks that allow screen dimming. Add WakeLockType::PreventDisplaySleepAllowDimming to the device service's WakeLock mojo interface. A similar level is currently exposed by chromeos::PowerPolicyController and used by ARC. This type is treated identically to PreventDisplaySleep on non-Chrome-OS platforms. Bug: 702449 Change-Id: I9810fa69c8472a264a067d34ee92cef0125d41e5 Reviewed-on: https://chromium-review.googlesource.com/749342 Reviewed-by: Greg Kerr <kerrnel@chromium.org> Reviewed-by: Ken Rockot <rockot@chromium.org> Commit-Queue: Dan Erat <derat@chromium.org> Cr-Commit-Position: refs/heads/master@{#513292} [modify] https://crrev.com/256b460474c11b858a12db6e15361eea0f7d3dc7/extensions/browser/api/power/power_api_unittest.cc [modify] https://crrev.com/256b460474c11b858a12db6e15361eea0f7d3dc7/services/device/public/interfaces/wake_lock.mojom [modify] https://crrev.com/256b460474c11b858a12db6e15361eea0f7d3dc7/services/device/wake_lock/power_save_blocker/power_save_blocker.h [modify] https://crrev.com/256b460474c11b858a12db6e15361eea0f7d3dc7/services/device/wake_lock/power_save_blocker/power_save_blocker_chromeos.cc [modify] https://crrev.com/256b460474c11b858a12db6e15361eea0f7d3dc7/services/device/wake_lock/power_save_blocker/power_save_blocker_mac.cc [modify] https://crrev.com/256b460474c11b858a12db6e15361eea0f7d3dc7/services/device/wake_lock/power_save_blocker/power_save_blocker_win.cc [modify] https://crrev.com/256b460474c11b858a12db6e15361eea0f7d3dc7/services/device/wake_lock/power_save_blocker/power_save_blocker_x11.cc [modify] https://crrev.com/256b460474c11b858a12db6e15361eea0f7d3dc7/services/device/wake_lock/wake_lock.cc [modify] https://crrev.com/256b460474c11b858a12db6e15361eea0f7d3dc7/services/device/wake_lock/wake_lock_unittest.cc
,
Nov 2 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a0b4d82d4932feada8671dd1b1bcb2cfad398e23 commit a0b4d82d4932feada8671dd1b1bcb2cfad398e23 Author: Daniel Erat <derat@chromium.org> Date: Thu Nov 02 20:39:58 2017 services/device: Add TestWakeLockProvider. Add a (partially-implemented) version of device::mojom::WakeLockProvider to make it easier to write unit tests for code that acquires and releases wake locks. Bug: 702449 Change-Id: Id4028efc0f228fb56e5cdc286689e255261b013c Reviewed-on: https://chromium-review.googlesource.com/750196 Reviewed-by: Ken Rockot <rockot@chromium.org> Commit-Queue: Dan Erat <derat@chromium.org> Cr-Commit-Position: refs/heads/master@{#513597} [add] https://crrev.com/a0b4d82d4932feada8671dd1b1bcb2cfad398e23/services/device/public/cpp/test/BUILD.gn [add] https://crrev.com/a0b4d82d4932feada8671dd1b1bcb2cfad398e23/services/device/public/cpp/test/test_wake_lock_provider.cc [add] https://crrev.com/a0b4d82d4932feada8671dd1b1bcb2cfad398e23/services/device/public/cpp/test/test_wake_lock_provider.h
,
Nov 4 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7f15c8b13d9629c551296c18c290d20b68474375 commit 7f15c8b13d9629c551296c18c290d20b68474375 Author: Daniel Erat <derat@chromium.org> Date: Sat Nov 04 00:12:41 2017 arc: Use device service wake locks. Make arc::ArcPowerBridge use device::mojom::WakeLock instead of calling directly into chromeos::PowerPolicyController. Bug: 702449 Change-Id: I328eb0b91f79d4866b73a15c56a61f8b37209a5c Reviewed-on: https://chromium-review.googlesource.com/748904 Commit-Queue: Dan Erat <derat@chromium.org> Reviewed-by: Antoine Labour <piman@chromium.org> Reviewed-by: Luis Hector Chavez <lhchavez@chromium.org> Reviewed-by: Ken Rockot <rockot@chromium.org> Cr-Commit-Position: refs/heads/master@{#513980} [modify] https://crrev.com/7f15c8b13d9629c551296c18c290d20b68474375/components/arc/BUILD.gn [modify] https://crrev.com/7f15c8b13d9629c551296c18c290d20b68474375/components/arc/power/DEPS [modify] https://crrev.com/7f15c8b13d9629c551296c18c290d20b68474375/components/arc/power/arc_power_bridge.cc [modify] https://crrev.com/7f15c8b13d9629c551296c18c290d20b68474375/components/arc/power/arc_power_bridge.h [add] https://crrev.com/7f15c8b13d9629c551296c18c290d20b68474375/components/arc/power/arc_power_bridge_unittest.cc
,
Nov 4 2017
,
Nov 6 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/fbefdb0bac773bb088f7145e24c6e582e7415aad commit fbefdb0bac773bb088f7145e24c6e582e7415aad Author: Daniel Erat <derat@chromium.org> Date: Mon Nov 06 21:27:20 2017 arc: Improve ArcPowerBridge testing. Add a FakePowerInstance class and make ArcPowerBridge tests instantiate chromeos::DBusThreadManager. Also add more tests: - Release device service wake locks when instance is closed. - Report suspend and resume to Android. - Set Android interactive state appropriately. - Synchronize screen brightness changes between Android and Chrome OS. Bug: 702449 Change-Id: I38276b76769fd9ad40ffa7eefb75d8a154f270c7 Reviewed-on: https://chromium-review.googlesource.com/754521 Commit-Queue: Dan Erat <derat@chromium.org> Reviewed-by: Luis Hector Chavez <lhchavez@chromium.org> Reviewed-by: Ken Rockot <rockot@chromium.org> Cr-Commit-Position: refs/heads/master@{#514244} [modify] https://crrev.com/fbefdb0bac773bb088f7145e24c6e582e7415aad/chromeos/dbus/fake_power_manager_client.cc [modify] https://crrev.com/fbefdb0bac773bb088f7145e24c6e582e7415aad/chromeos/dbus/fake_power_manager_client.h [modify] https://crrev.com/fbefdb0bac773bb088f7145e24c6e582e7415aad/components/arc/BUILD.gn [modify] https://crrev.com/fbefdb0bac773bb088f7145e24c6e582e7415aad/components/arc/power/arc_power_bridge.cc [modify] https://crrev.com/fbefdb0bac773bb088f7145e24c6e582e7415aad/components/arc/power/arc_power_bridge.h [modify] https://crrev.com/fbefdb0bac773bb088f7145e24c6e582e7415aad/components/arc/power/arc_power_bridge_unittest.cc [add] https://crrev.com/fbefdb0bac773bb088f7145e24c6e582e7415aad/components/arc/test/fake_power_instance.cc [add] https://crrev.com/fbefdb0bac773bb088f7145e24c6e582e7415aad/components/arc/test/fake_power_instance.h [modify] https://crrev.com/fbefdb0bac773bb088f7145e24c6e582e7415aad/services/device/public/cpp/test/test_wake_lock_provider.cc [modify] https://crrev.com/fbefdb0bac773bb088f7145e24c6e582e7415aad/services/device/public/cpp/test/test_wake_lock_provider.h
,
Nov 7 2017
,
Feb 26 2018
,
Aug 15
,
Oct 17
|
||||||||||
►
Sign in to add a comment |
||||||||||
Comment 1 by derat@chromium.org
, Mar 17 2017