Issue 12186010: chromeos: Add power management policy prefs.

Issue 12186010: chromeos: Add power management policy prefs. (Closed)

Created:
7 years, 10 months ago by Daniel Erat

Modified:
7 years, 10 months ago

Reviewers:
bartfab (slow), Mattias Nissler (ping if slow)

CC:
chromium-reviews, oshima+watch_chromium.org, stevenjb+watch_chromium.org, nkostylev+watch_chromium.org, davemoore+watch_chromium.org

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Visibility:
Public.

More Reviews

Description

chromeos: Add power management policy prefs. This adds various prefs related to power management. A new PowerPolicyController class is responsible for sending them to powerd. BUG=173849, chromium-os:38281 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=181070

Patch Set 1 #

Total comments: 27

Patch Set 2 : pass prefs directly to PowerPolicyController #

Total comments: 10

Patch Set 3 : update comments and add missing include #

Patch Set 4 : fix compilation :-/ #

Patch Set 5 : merge #

Patch Set 6 : fix compilation of mocks, i think #

Total comments: 8

Patch Set 7 : merge #

Patch Set 8 : resend policy if power manager restarts #

Patch Set 9 : fix embarrassing precedence issue in macro #

Patch Set 10 : fix crash at shutdown, hopefully #

Patch Set 11 : fix observer registration and bool prefs #

Created: 7 years, 10 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+495 lines, -5 lines)			Patch
M	chrome/browser/chromeos/preferences.h	View	1	1 chunk	+15 lines, -0 lines	0 comments	Download
M	chrome/browser/chromeos/preferences.cc	View	1 2 3 4 5 6 7 8 9 10	4 chunks	+103 lines, -0 lines	0 comments	Download
M	chrome/common/pref_names.h	View	1 2 3 4	1 chunk	+13 lines, -0 lines	0 comments	Download
M	chrome/common/pref_names.cc	View	1 2 3 4	1 chunk	+36 lines, -0 lines	0 comments	Download
M	chromeos/chromeos.gyp	View	1 2 3 4 5	3 chunks	+4 lines, -0 lines	0 comments	Download
M	chromeos/dbus/dbus_thread_manager.h	View		2 chunks	+2 lines, -0 lines	0 comments	Download
M	chromeos/dbus/dbus_thread_manager.cc	View	1 2 3 4 5 6 7 8 9 10	5 chunks	+15 lines, -2 lines	0 comments	Download
M	chromeos/dbus/mock_dbus_thread_manager.h	View	1 2 3 4 5 6 7 8 9 10	4 chunks	+7 lines, -2 lines	0 comments	Download
M	chromeos/dbus/mock_dbus_thread_manager.cc	View	1 2 3 4 5 6 7 8 9 10	3 chunks	+6 lines, -1 line	0 comments	Download
M	chromeos/dbus/mock_dbus_thread_manager_without_gmock.h	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	chromeos/dbus/mock_dbus_thread_manager_without_gmock.cc	View		1 chunk	+6 lines, -0 lines	0 comments	Download
M	chromeos/dbus/mock_power_manager_client.h	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	chromeos/dbus/mock_power_manager_client.cc	View	1 2 3 4 5	1 chunk	+1 line, -0 lines	0 comments	Download
M	chromeos/dbus/power_manager_client.h	View	1 2 3 4 5 6	2 chunks	+8 lines, -0 lines	0 comments	Download
M	chromeos/dbus/power_manager_client.cc	View	1 2 3 4 5 6	4 chunks	+20 lines, -0 lines	0 comments	Download
A	chromeos/dbus/power_policy_controller.h	View	1 2 3 4 5 6 7 8 9 10	1 chunk	+79 lines, -0 lines	0 comments	Download
A	chromeos/dbus/power_policy_controller.cc	View	1 2 3 4 5 6 7 8 9 10	1 chunk	+178 lines, -0 lines	0 comments	Download

Messages

Total messages: 17 (0 generated)

Expand Messages | Collapse Messages

Daniel Erat

I haven't tested this at all yet, but I figured I'd send it your way ...

7 years, 10 months ago (2013-02-04 03:10:07 UTC) #1

bartfab (slow)

https://codereview.chromium.org/12186010/diff/1/chrome/browser/chromeos/preferences.cc File chrome/browser/chromeos/preferences.cc (right): https://codereview.chromium.org/12186010/diff/1/chrome/browser/chromeos/preferences.cc#newcode303 chrome/browser/chromeos/preferences.cc:303: chromeos::PowerPolicyController::kUnsetDelayMs, You do not need magic value constants such ...

7 years, 10 months ago (2013-02-04 10:04:35 UTC) #2

Daniel Erat

7 years, 10 months ago (2013-02-04 13:35:45 UTC) #3

https://codereview.chromium.org/12186010/diff/1/chrome/browser/chromeos/prefe...
File chrome/browser/chromeos/preferences.cc (right):

https://codereview.chromium.org/12186010/diff/1/chrome/browser/chromeos/prefe...
chrome/browser/chromeos/preferences.cc:303:
chromeos::PowerPolicyController::kUnsetDelayMs,
On 2013/02/04 10:04:35, bartfab wrote:
> You do not need magic value constants such as "kUnsetDelayMs", "ACTION_UNSET",
> "BOOL_UNSET" to detect if a pref has been set. You should just set the prefs
to
> their actual default state in this file. In the code that sends power
management
> policy to powerd, you can then use
PrefServiceBase::Preference::IsDefaultValue()
> to check whether the pref is still at its default or not.

Sure, I'll update this.  I didn't realize that IsDefaultValue() would
differentiate between unset prefs and set-to-the-same-value-as-the-default
prefs, but I now see that that's the case.

> When we decide to make these prefs available in the UI, we may run into a
> problem here: The default values shown to the user should match the defaults
on
> the current platform. But you mentioned that the defaults may vary per board.
I
> guess we will need to pass them into Chrome somehow.

I'm not sure how many of these, if any, we'll want to make available in the UI. 
My longer-term intent is for the default values to live in Chrome instead of in
powerd, though, i.e. Chrome should always be passing values to powerd instead of
leaving them unset.

https://codereview.chromium.org/12186010/diff/1/chrome/browser/chromeos/prefe...
File chrome/browser/chromeos/preferences.h (right):

https://codereview.chromium.org/12186010/diff/1/chrome/browser/chromeos/prefe...
chrome/browser/chromeos/preferences.h:101: void UpdatePowerManagementPolicy();
On 2013/02/04 10:04:35, bartfab wrote:
> It's a bit unfortunate that this is called "policy" as Chrome has a notion of
> "policy" and it means something different.
> 
> Could you disambiguate in the comment a bit by using e.g. "power manager
policy"
> or "powerd policy"?

Not sure what you mean here -- this is already called
"UpdatePowerManagementPolicy()".  Do you mean you want it to be
"UpdatePowerManagerPolicy()" instead?

https://codereview.chromium.org/12186010/diff/1/chromeos/dbus/mock_dbus_threa...
File chromeos/dbus/mock_dbus_thread_manager.cc (right):

https://codereview.chromium.org/12186010/diff/1/chromeos/dbus/mock_dbus_threa...
chromeos/dbus/mock_dbus_thread_manager.cc:66: power_policy_controller_(
On 2013/02/04 10:04:35, bartfab wrote:
> I haven't looked at what gets mock here in detail but I presume there is no
need
> to mock the PowerPolicyController because all its DBus calls go through the
> PowerManagerClient, which has been mocked?

Yes.

https://codereview.chromium.org/12186010/diff/1/chromeos/dbus/power_policy_co...
File chromeos/dbus/power_policy_controller.h (right):

https://codereview.chromium.org/12186010/diff/1/chromeos/dbus/power_policy_co...
chromeos/dbus/power_policy_controller.h:21: ACTION_UNSET        = -1,
On 2013/02/04 10:04:35, bartfab wrote:
> This value is not needed.

I added this because the pref code needed to use it to tell
PowerPolicyController that a given pref was unset.  See below, though -- I agree
that it's not needed if the prefs are passed directly into this class.

https://codereview.chromium.org/12186010/diff/1/chromeos/dbus/power_policy_co...
chromeos/dbus/power_policy_controller.h:64: void UpdatePolicyFromPrefs(const
PolicyPrefs& prefs);
On 2013/02/04 10:04:35, bartfab wrote:
> On 2013/02/04 03:10:07, Daniel Erat wrote:
> > (My intent is that this class will later become responsible for additional
> > policy changes initiating within Chrome, such as blocking idle suspend
before
> > the OOBE has been completed.  It'll need to combine those changes with the
> > settings loaded from prefs.)
> 
> Why not reference the actual prefs? The PolicyPrefs class ends up duplicating
> the values in memory that the actual prefs already hold.

I didn't realize that the prefs code derives from an interface defined in base. 
Just to make sure that we're in agreement, you're suggesting that this method
instead take a bunch of PrefServiceBase::Preference (references?), right?  I
agree that that's much simpler.

bartfab (slow)

7 years, 10 months ago (2013-02-04 14:43:51 UTC) #4

https://codereview.chromium.org/12186010/diff/1/chrome/browser/chromeos/prefe...
File chrome/browser/chromeos/preferences.cc (right):

https://codereview.chromium.org/12186010/diff/1/chrome/browser/chromeos/prefe...
chrome/browser/chromeos/preferences.cc:303:
chromeos::PowerPolicyController::kUnsetDelayMs,
On 2013/02/04 13:35:45, Daniel Erat wrote:
> On 2013/02/04 10:04:35, bartfab wrote:
> > You do not need magic value constants such as "kUnsetDelayMs",
"ACTION_UNSET",
> > "BOOL_UNSET" to detect if a pref has been set. You should just set the prefs
> to
> > their actual default state in this file. In the code that sends power
> management
> > policy to powerd, you can then use
> PrefServiceBase::Preference::IsDefaultValue()
> > to check whether the pref is still at its default or not.
> 
> Sure, I'll update this.  I didn't realize that IsDefaultValue() would
> differentiate between unset prefs and set-to-the-same-value-as-the-default
> prefs, but I now see that that's the case.
> 
> > When we decide to make these prefs available in the UI, we may run into a
> > problem here: The default values shown to the user should match the defaults
> on
> > the current platform. But you mentioned that the defaults may vary per
board.
> I
> > guess we will need to pass them into Chrome somehow.
> 
> I'm not sure how many of these, if any, we'll want to make available in the
UI. 
> My longer-term intent is for the default values to live in Chrome instead of
in
> powerd, though, i.e. Chrome should always be passing values to powerd instead
of
> leaving them unset.

That's perfect. And yes, the issue will not arise for now anyway as we are not
exposing any of the settings in the UI.

https://codereview.chromium.org/12186010/diff/1/chrome/browser/chromeos/prefe...
File chrome/browser/chromeos/preferences.h (right):

https://codereview.chromium.org/12186010/diff/1/chrome/browser/chromeos/prefe...
chrome/browser/chromeos/preferences.h:101: void UpdatePowerManagementPolicy();
On 2013/02/04 13:35:45, Daniel Erat wrote:
> On 2013/02/04 10:04:35, bartfab wrote:
> > It's a bit unfortunate that this is called "policy" as Chrome has a notion
of
> > "policy" and it means something different.
> > 
> > Could you disambiguate in the comment a bit by using e.g. "power manager
> policy"
> > or "powerd policy"?
> 
> Not sure what you mean here -- this is already called
> "UpdatePowerManagementPolicy()".  Do you mean you want it to be
> "UpdatePowerManagerPolicy()" instead?

Yes, I was thinking of PowerManagerPolicy. That way, it should be a tad clearer
that this is not "policy" in the Chrome sense but policy in the sense of some
other entity, the power manager.

https://codereview.chromium.org/12186010/diff/1/chromeos/dbus/power_policy_co...
File chromeos/dbus/power_policy_controller.h (right):

https://codereview.chromium.org/12186010/diff/1/chromeos/dbus/power_policy_co...
chromeos/dbus/power_policy_controller.h:64: void UpdatePolicyFromPrefs(const
PolicyPrefs& prefs);
On 2013/02/04 13:35:45, Daniel Erat wrote:
> On 2013/02/04 10:04:35, bartfab wrote:
> > On 2013/02/04 03:10:07, Daniel Erat wrote:
> > > (My intent is that this class will later become responsible for additional
> > > policy changes initiating within Chrome, such as blocking idle suspend
> before
> > > the OOBE has been completed.  It'll need to combine those changes with the
> > > settings loaded from prefs.)
> > 
> > Why not reference the actual prefs? The PolicyPrefs class ends up
duplicating
> > the values in memory that the actual prefs already hold.
> 
> I didn't realize that the prefs code derives from an interface defined in
base. 
> Just to make sure that we're in agreement, you're suggesting that this method
> instead take a bunch of PrefServiceBase::Preference (references?), right?  I
> agree that that's much simpler.

Either pass a number of Preferences to this class or, even simpler, give it a
pointer to a PrefServiceBase and let the class request whatever Preferences and
observers it needs. Since Preferences members automatically update and observers
automatically fire when prefs change, you will no longer need to ping the class
on each update either. It should pick up pref changes automagically.

bartfab (slow)

https://codereview.chromium.org/12186010/diff/18/chrome/common/pref_names.cc File chrome/common/pref_names.cc (right): https://codereview.chromium.org/12186010/diff/18/chrome/common/pref_names.cc#newcode832 chrome/common/pref_names.cc:832: // kPowerAcIdleDelayMs); -1 instructs the power manager to use ...

7 years, 10 months ago (2013-02-04 17:20:05 UTC) #6

Daniel Erat

Thanks (and sorry for missing all those comments). https://codereview.chromium.org/12186010/diff/18/chrome/common/pref_names.cc File chrome/common/pref_names.cc (right): https://codereview.chromium.org/12186010/diff/18/chrome/common/pref_names.cc#newcode832 chrome/common/pref_names.cc:832: // ...

7 years, 10 months ago (2013-02-04 17:31:59 UTC) #7

bartfab (slow)

Loking pretty good. I am adding Mattias to get another opinion on whether we need ...

7 years, 10 months ago (2013-02-04 17:46:36 UTC) #8

Mattias Nissler (ping if slow)

Looks reasonable to me too. I'm actually not too concerned about the "policy" name clash ...

7 years, 10 months ago (2013-02-05 13:16:18 UTC) #9

Daniel Erat

https://codereview.chromium.org/12186010/diff/17001/chromeos/dbus/mock_power_manager_client.cc File chromeos/dbus/mock_power_manager_client.cc (right): https://codereview.chromium.org/12186010/diff/17001/chromeos/dbus/mock_power_manager_client.cc#newcode6 chromeos/dbus/mock_power_manager_client.cc:6: #include "chromeos/dbus/power_manager/policy.pb.h" On 2013/02/05 13:16:18, Mattias Nissler wrote: > ...

7 years, 10 months ago (2013-02-05 14:05:58 UTC) #10

Daniel Erat

So... is your hope still to use this for Public Accounts in 26? <looking anxiously ...

7 years, 10 months ago (2013-02-05 20:41:35 UTC) #11

bartfab (slow)

Yes, the hope still is to get this into M26. If your CL successfully lands ...

7 years, 10 months ago (2013-02-05 21:13:26 UTC) #12

Daniel Erat

Thanks. I'm probably going to hold off on checking this in until I can do ...

7 years, 10 months ago (2013-02-06 00:43:36 UTC) #13

bartfab (slow)

Actually, things work the other way around. First, you add prefs (which the user can ...

7 years, 10 months ago (2013-02-06 15:32:20 UTC) #14

Actually, things work the other way around. First, you add prefs (which the user
can freely modify as they reside in a file named "Preferences" in the user's
profile directory). Then, we add policies that can override the user's settings
with mandatory values.

To test your current implementation, log in as a user, modify the Preferences
file in the user's home directory and restart Chrome. The browser should pick up
the new policy value and should start generating the right power management
events. When you land your CL and the prefs are there, I will then add policies
and will test against a test DM server. Right now, a test DM server would do
nothing as there are no policies in place yet.

In summary, essentially, you can ignore the entire enterprise world in your
testing. As long as the prefs cause the right thing to happen, we are good.
Policies will simply manipulate the pref values. No more, no less.

On 2013/02/06 00:43:36, Daniel Erat wrote:
> Thanks.  I'm probably going to hold off on checking this in until I can do
> a bit more manual testing (e.g. setting the prefs via a test DM server and
> checking that the correct policy shows up in powerd), so it might not land
> until tomorrow PST.  I don't expect any big changes to it, so perhaps you
> can patch it into your local client while working on the policies.  (I also
> have no experience with enterprise stuff, so if it's easy for you to test
> this and check that the expected settings should up in
> /var/log/power_manager/powerd.LATEST, please do!)
> 
> On Tue, Feb 5, 2013 at 1:13 PM, <mailto:bartfab@chromium.org> wrote:
> 
> > Yes, the hope still is to get this into M26. If your CL successfully lands
> > tonight, I can add the policies tomorrow.
> >
> > LGTM by the way. And as Mattias only had a few minor comments, I believe
> > he is
> > OK with the CL as well (it's night EMEA time, so do not expect any further
> > response from us until tomorrow).
> >
> >
>
https://codereview.chromium.**org/12186010/%3Chttps://codereview.chromium.org...>
> >

Daniel Erat

On 2013/02/06 15:32:20, bartfab wrote: > Actually, things work the other way around. First, you ...

7 years, 10 months ago (2013-02-06 16:12:11 UTC) #15

On 2013/02/06 15:32:20, bartfab wrote:
> Actually, things work the other way around. First, you add prefs (which the
user
> can freely modify as they reside in a file named "Preferences" in the user's
> profile directory). Then, we add policies that can override the user's
settings
> with mandatory values.
> 
> To test your current implementation, log in as a user, modify the Preferences
> file in the user's home directory and restart Chrome. The browser should pick
up
> the new policy value and should start generating the right power management
> events. When you land your CL and the prefs are there, I will then add
policies
> and will test against a test DM server. Right now, a test DM server would do
> nothing as there are no policies in place yet.
> 
> In summary, essentially, you can ignore the entire enterprise world in your
> testing. As long as the prefs cause the right thing to happen, we are good.
> Policies will simply manipulate the pref values. No more, no less.

Great, thanks for the explanation!  That's what I was doing yesterday to test
this.  My question was with regard to exercising the code in the Preferences
class that tries to watch for changes to the prefs while Chrome is running, but
perhaps it's simple/boilerplate enough that it's fine to check it in.

> On 2013/02/06 00:43:36, Daniel Erat wrote:
> > Thanks.  I'm probably going to hold off on checking this in until I can do
> > a bit more manual testing (e.g. setting the prefs via a test DM server and
> > checking that the correct policy shows up in powerd), so it might not land
> > until tomorrow PST.  I don't expect any big changes to it, so perhaps you
> > can patch it into your local client while working on the policies.  (I also
> > have no experience with enterprise stuff, so if it's easy for you to test
> > this and check that the expected settings should up in
> > /var/log/power_manager/powerd.LATEST, please do!)
> > 
> > On Tue, Feb 5, 2013 at 1:13 PM, <mailto:bartfab@chromium.org> wrote:
> > 
> > > Yes, the hope still is to get this into M26. If your CL successfully lands
> > > tonight, I can add the policies tomorrow.
> > >
> > > LGTM by the way. And as Mattias only had a few minor comments, I believe
> > > he is
> > > OK with the CL as well (it's night EMEA time, so do not expect any further
> > > response from us until tomorrow).
> > >
> > >
> >
>
https://codereview.chromium.**org/12186010/%253Chttps://codereview.chromium.o...>
> > >

bartfab (slow)

On 2013/02/06 16:12:11, Daniel Erat wrote: > On 2013/02/06 15:32:20, bartfab wrote: > > Actually, ...

7 years, 10 months ago (2013-02-06 16:14:19 UTC) #16

On 2013/02/06 16:12:11, Daniel Erat wrote:
> On 2013/02/06 15:32:20, bartfab wrote:
> > Actually, things work the other way around. First, you add prefs (which the
> user
> > can freely modify as they reside in a file named "Preferences" in the user's
> > profile directory). Then, we add policies that can override the user's
> settings
> > with mandatory values.
> > 
> > To test your current implementation, log in as a user, modify the
Preferences
> > file in the user's home directory and restart Chrome. The browser should
pick
> up
> > the new policy value and should start generating the right power management
> > events. When you land your CL and the prefs are there, I will then add
> policies
> > and will test against a test DM server. Right now, a test DM server would do
> > nothing as there are no policies in place yet.
> > 
> > In summary, essentially, you can ignore the entire enterprise world in your
> > testing. As long as the prefs cause the right thing to happen, we are good.
> > Policies will simply manipulate the pref values. No more, no less.
> 
> Great, thanks for the explanation!  That's what I was doing yesterday to test
> this.  My question was with regard to exercising the code in the Preferences
> class that tries to watch for changes to the prefs while Chrome is running,
but
> perhaps it's simple/boilerplate enough that it's fine to check it in.

Yeah, there is no good way to exercise this short of adding policies. And I am
still busy with other M26 features, so I have not had a chance to add policies
and test with these. It is all boilerplate stuff though, so it should Just Work.

> 
> > On 2013/02/06 00:43:36, Daniel Erat wrote:
> > > Thanks.  I'm probably going to hold off on checking this in until I can do
> > > a bit more manual testing (e.g. setting the prefs via a test DM server and
> > > checking that the correct policy shows up in powerd), so it might not land
> > > until tomorrow PST.  I don't expect any big changes to it, so perhaps you
> > > can patch it into your local client while working on the policies.  (I
also
> > > have no experience with enterprise stuff, so if it's easy for you to test
> > > this and check that the expected settings should up in
> > > /var/log/power_manager/powerd.LATEST, please do!)
> > > 
> > > On Tue, Feb 5, 2013 at 1:13 PM, <mailto:bartfab@chromium.org> wrote:
> > > 
> > > > Yes, the hope still is to get this into M26. If your CL successfully
lands
> > > > tonight, I can add the policies tomorrow.
> > > >
> > > > LGTM by the way. And as Mattias only had a few minor comments, I believe
> > > > he is
> > > > OK with the CL as well (it's night EMEA time, so do not expect any
further
> > > > response from us until tomorrow).
> > > >
> > > >
> > >
> >
>
https://codereview.chromium.**org/12186010/%25253Chttps://codereview.chromium...>
> > > >

commit-bot: I haz the power

7 years, 10 months ago (2013-02-06 20:36:35 UTC) #17

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/derat@chromium.org/12186010/24004

Expand Messages | Collapse Messages