Add policies to control power management on the Chrome OS login screen

Add policies to control power management on the Chrome OS login screen

This CL adds a device policy that controls power management on the login
screen. Before this CL, powerd controlled power policy on the login screen,
always using built-in defaults. After this CL, Chrome contols power policy
by explicitly sending it to powerd whenever the login screen is shown. The
built-in defaults match those that powerd used to have but they can now be
overridden through device policy.

BUG=241794
TEST=Full coverage with new unit and broweser tests

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

[bartfab (slow), 2013-06-28 16:14:16]

Hi Daniel,

Could you please review the */chromeos/* parts of this CL?

Hi Mattias,

Could you please review the policy part of this CL?

Hi Scott,

Could you please review the testing_profile_manager.[cc|h] changes?

[Daniel Erat, 2013-06-28 17:05:09]

LGTM with some comments for the chrome os parts

https://codereview.chromium.org/18153007/diff/1/chrome/browser/chromeos/power...
File chrome/browser/chromeos/power/power_prefs.cc (right):

https://codereview.chromium.org/18153007/diff/1/chrome/browser/chromeos/power...
chrome/browser/chromeos/power/power_prefs.cc:92: for_signin_profile ?
PowerPolicyController::ACTION_SHUT_DOWN :
just to double-check: for_signin_profile will be false in guest mode, right?

https://codereview.chromium.org/18153007/diff/1/chrome/browser/chromeos/power...
chrome/browser/chromeos/power/power_prefs.cc:113: prefs::kEnableScreenLock,
i'm not entirely convinced that this should be considered a
power-management-related pref (even though it's used by PowerPolicyController).
do you feel strongly one way or the other about it?

https://codereview.chromium.org/18153007/diff/1/chrome/browser/chromeos/power...
chrome/browser/chromeos/power/power_prefs.cc:144: }
do you need a default: NOTREACHED() here to keep clang from complaining?

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

https://codereview.chromium.org/18153007/diff/1/chrome/browser/chromeos/power...
chrome/browser/chromeos/power/power_prefs.h:25: class PowerPrefs : public
content::NotificationObserver {
nit: add a comment describing what this class does

https://codereview.chromium.org/18153007/diff/1/chrome/browser/chromeos/power...
File chrome/browser/chromeos/power/power_prefs_unittest.cc (right):

https://codereview.chromium.org/18153007/diff/1/chrome/browser/chromeos/power...
chrome/browser/chromeos/power/power_prefs_unittest.cc:81: void
PowerPrefsTest::VerifyProfileIsBeingUsed(Profile* profile) {
to make test failures have meaningful line numbers, i'd prefer that you instead
did something like this:

  std::string GetExpectedPolicyForProfile(Profile* profile) {
    ...
    return PowerpolicyController::GetPolicyDebugString(expected_policy);
  }

  EXPECT_EQ(GetExpectedPolicyForProfile(profile),
            PowerPolicyController::GetPolicyDebugString(
                fake_power_manager_client_->get_policy()));

if you'd rather not do that, please put all of the Verify*() calls below into
SCOPED_TRACE() blocks

https://codereview.chromium.org/18153007/diff/1/chrome/browser/chromeos/power...
chrome/browser/chromeos/power/power_prefs_unittest.cc:166: // Verify that the
login profile's power prefs are being used.
i'd probably delete the rest of the comments in this test fixture; they don't
seem to add much

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

https://codereview.chromium.org/18153007/diff/1/chromeos/dbus/power_policy_co...
chromeos/dbus/power_policy_controller.cc:160: lock_ms < delays->idle_ms()) {
for what it's worth, i've been told in previous reviews that curly brackets
should be left off in this case. i don't care much one way or the other

[sky, 2013-06-28 18:06:20]

LGTM

[bartfab (slow), 2013-07-01 12:32:58]

https://codereview.chromium.org/18153007/diff/1/chrome/browser/chromeos/power...
File chrome/browser/chromeos/power/power_prefs.cc (right):

https://codereview.chromium.org/18153007/diff/1/chrome/browser/chromeos/power...
chrome/browser/chromeos/power/power_prefs.cc:92: for_signin_profile ?
PowerPolicyController::ACTION_SHUT_DOWN :
On 2013/06/28 17:05:09, Daniel Erat wrote:
> just to double-check: for_signin_profile will be false in guest mode, right?

Yes. The signin profile is a special profile used by the login screen / OOBE
mechanism only. The guest profile and all user profiles are entirely separate
from it.

https://codereview.chromium.org/18153007/diff/1/chrome/browser/chromeos/power...
chrome/browser/chromeos/power/power_prefs.cc:113: prefs::kEnableScreenLock,
On 2013/06/28 17:05:09, Daniel Erat wrote:
> i'm not entirely convinced that this should be considered a
> power-management-related pref (even though it's used by
PowerPolicyController).
> do you feel strongly one way or the other about it?

I do not feel strongly either way but considering the fact that the screen lock
pref is only actually read and used by the power management code, I think it
belongs here more than anywhere else.

https://codereview.chromium.org/18153007/diff/1/chrome/browser/chromeos/power...
chrome/browser/chromeos/power/power_prefs.cc:144: }
On 2013/06/28 17:05:09, Daniel Erat wrote:
> do you need a default: NOTREACHED() here to keep clang from complaining?

I had not tried to compile this with clang yet. Makes sense to add a default:
NOTREACHED() to catch errors either way. Done.

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

https://codereview.chromium.org/18153007/diff/1/chrome/browser/chromeos/power...
chrome/browser/chromeos/power/power_prefs.h:25: class PowerPrefs : public
content::NotificationObserver {
On 2013/06/28 17:05:09, Daniel Erat wrote:
> nit: add a comment describing what this class does

Done.

https://codereview.chromium.org/18153007/diff/1/chrome/browser/chromeos/power...
File chrome/browser/chromeos/power/power_prefs_unittest.cc (right):

https://codereview.chromium.org/18153007/diff/1/chrome/browser/chromeos/power...
chrome/browser/chromeos/power/power_prefs_unittest.cc:81: void
PowerPrefsTest::VerifyProfileIsBeingUsed(Profile* profile) {
On 2013/06/28 17:05:09, Daniel Erat wrote:
> to make test failures have meaningful line numbers, i'd prefer that you
instead
> did something like this:
> 
>   std::string GetExpectedPolicyForProfile(Profile* profile) {
>     ...
>     return PowerpolicyController::GetPolicyDebugString(expected_policy);
>   }
> 
>   EXPECT_EQ(GetExpectedPolicyForProfile(profile),
>             PowerPolicyController::GetPolicyDebugString(
>                 fake_power_manager_client_->get_policy()));
> 
> if you'd rather not do that, please put all of the Verify*() calls below into
> SCOPED_TRACE() blocks

Done.

https://codereview.chromium.org/18153007/diff/1/chrome/browser/chromeos/power...
chrome/browser/chromeos/power/power_prefs_unittest.cc:166: // Verify that the
login profile's power prefs are being used.
On 2013/06/28 17:05:09, Daniel Erat wrote:
> i'd probably delete the rest of the comments in this test fixture; they don't
> seem to add much

I snipped some of the comments. I think the bit about the other, unrelated
profile is not self-explanatory though. I left some comments that explain that
the test destroys an unrelated profile and verifies that this does not affect
the state of the power policy.

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

https://codereview.chromium.org/18153007/diff/1/chromeos/dbus/power_policy_co...
chromeos/dbus/power_policy_controller.cc:160: lock_ms < delays->idle_ms()) {
On 2013/06/28 17:05:09, Daniel Erat wrote:
> for what it's worth, i've been told in previous reviews that curly brackets
> should be left off in this case. i don't care much one way or the other

The style guide definitely does not forbid them:

"In general, curly braces are not required for single-line statements, but they
are allowed if you like them; conditional or loop statements with complex
conditions or statements may be more readable with curly braces. Some projects
require that an if must always always have an accompanying brace."

And I have been told in previous reviews that curly braces would be required
here as it is seen as a multi-line statement. Given the choice, I would prefer
to keep them.

[Daniel Erat, 2013-07-01 14:29:05]

lgtm

https://codereview.chromium.org/18153007/diff/1/chrome/browser/chromeos/power...
File chrome/browser/chromeos/power/power_prefs.cc (right):

https://codereview.chromium.org/18153007/diff/1/chrome/browser/chromeos/power...
chrome/browser/chromeos/power/power_prefs.cc:113: prefs::kEnableScreenLock,
On 2013/07/01 12:32:59, bartfab wrote:
> On 2013/06/28 17:05:09, Daniel Erat wrote:
> > i'm not entirely convinced that this should be considered a
> > power-management-related pref (even though it's used by
> PowerPolicyController).
> > do you feel strongly one way or the other about it?
> 
> I do not feel strongly either way but considering the fact that the screen
lock
> pref is only actually read and used by the power management code, I think it
> belongs here more than anywhere else.

okay, fine by me

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

https://codereview.chromium.org/18153007/diff/1/chromeos/dbus/power_policy_co...
chromeos/dbus/power_policy_controller.cc:160: lock_ms < delays->idle_ms()) {
On 2013/07/01 12:32:59, bartfab wrote:
> On 2013/06/28 17:05:09, Daniel Erat wrote:
> > for what it's worth, i've been told in previous reviews that curly brackets
> > should be left off in this case. i don't care much one way or the other
> 
> The style guide definitely does not forbid them:
> 
> "In general, curly braces are not required for single-line statements, but
they
> are allowed if you like them; conditional or loop statements with complex
> conditions or statements may be more readable with curly braces. Some projects
> require that an if must always always have an accompanying brace."
> 
> And I have been told in previous reviews that curly braces would be required
> here as it is seen as a multi-line statement. Given the choice, I would prefer
> to keep them.

sounds fine. for what it's worth, the distinction that i've heard is that a
multi-line statement requires braces, while a multi-line condition with a
single-line statement shouldn't have them:

if (SomeVeryLongConditionIsTrue() &&
    SomeOtherVeryLongConditionIsTrue())
  DoSomething();

if (ConditionIsTrue) {
  DoSomethingWithALotOfArguments(
      this_variable_name_seems_awfully_long,
      really_cant_you_come_up_with_something_shorter,
      it_is_not_like_we_are_paid_by_the_line_after_all);
}

but i don't think that this has ever been spelled out in a style guide, and i'm
not interested in wading through years of style discussions on chromium-dev to
see if it's ever actually come up there. :-P

https://codereview.chromium.org/18153007/diff/12001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/power/power_prefs.h (right):

https://codereview.chromium.org/18153007/diff/12001/chrome/browser/chromeos/p...
chrome/browser/chromeos/power/power_prefs.h:28: // of the power-related prefs
changes.
nit: the style guide implies that class comments should be above the class
definition rather than above the c'tor:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Class_...

[bartfab (slow), 2013-07-01 14:53:07]

https://chromiumcodereview.appspot.com/18153007/diff/1/chromeos/dbus/power_po...
File chromeos/dbus/power_policy_controller.cc (right):

https://chromiumcodereview.appspot.com/18153007/diff/1/chromeos/dbus/power_po...
chromeos/dbus/power_policy_controller.cc:160: lock_ms < delays->idle_ms()) {
On 2013/07/01 14:29:05, Daniel Erat wrote:
> On 2013/07/01 12:32:59, bartfab wrote:
> > On 2013/06/28 17:05:09, Daniel Erat wrote:
> > > for what it's worth, i've been told in previous reviews that curly
brackets
> > > should be left off in this case. i don't care much one way or the other
> > 
> > The style guide definitely does not forbid them:
> > 
> > "In general, curly braces are not required for single-line statements, but
> they
> > are allowed if you like them; conditional or loop statements with complex
> > conditions or statements may be more readable with curly braces. Some
projects
> > require that an if must always always have an accompanying brace."
> > 
> > And I have been told in previous reviews that curly braces would be required
> > here as it is seen as a multi-line statement. Given the choice, I would
prefer
> > to keep them.
> 
> sounds fine. for what it's worth, the distinction that i've heard is that a
> multi-line statement requires braces, while a multi-line condition with a
> single-line statement shouldn't have them:
> 
> if (SomeVeryLongConditionIsTrue() &&
>     SomeOtherVeryLongConditionIsTrue())
>   DoSomething();
> 
> if (ConditionIsTrue) {
>   DoSomethingWithALotOfArguments(
>       this_variable_name_seems_awfully_long,
>       really_cant_you_come_up_with_something_shorter,
>       it_is_not_like_we_are_paid_by_the_line_after_all);
> }
> 
> but i don't think that this has ever been spelled out in a style guide, and
i'm
> not interested in wading through years of style discussions on chromium-dev to
> see if it's ever actually come up there. :-P

It seems that different reviewers interpret the style guide differently. If you
are OK with the code as it is, I will just leave it like that rather than
starting a bikeshed discussion on chromium-dev :).

https://chromiumcodereview.appspot.com/18153007/diff/12001/chrome/browser/chr...
File chrome/browser/chromeos/power/power_prefs.h (right):

https://chromiumcodereview.appspot.com/18153007/diff/12001/chrome/browser/chr...
chrome/browser/chromeos/power/power_prefs.h:28: // of the power-related prefs
changes.
On 2013/07/01 14:29:05, Daniel Erat wrote:
> nit: the style guide implies that class comments should be above the class
> definition rather than above the c'tor:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Class_...

Oops. No idea how the comment slipped down here. Done.

[Daniel Erat, 2013-07-01 14:53:52]

https://chromiumcodereview.appspot.com/18153007/diff/1/chromeos/dbus/power_po...
File chromeos/dbus/power_policy_controller.cc (right):

https://chromiumcodereview.appspot.com/18153007/diff/1/chromeos/dbus/power_po...
chromeos/dbus/power_policy_controller.cc:160: lock_ms < delays->idle_ms()) {
On 2013/07/01 14:53:08, bartfab wrote:
> On 2013/07/01 14:29:05, Daniel Erat wrote:
> > On 2013/07/01 12:32:59, bartfab wrote:
> > > On 2013/06/28 17:05:09, Daniel Erat wrote:
> > > > for what it's worth, i've been told in previous reviews that curly
> brackets
> > > > should be left off in this case. i don't care much one way or the other
> > > 
> > > The style guide definitely does not forbid them:
> > > 
> > > "In general, curly braces are not required for single-line statements, but
> > they
> > > are allowed if you like them; conditional or loop statements with complex
> > > conditions or statements may be more readable with curly braces. Some
> projects
> > > require that an if must always always have an accompanying brace."
> > > 
> > > And I have been told in previous reviews that curly braces would be
required
> > > here as it is seen as a multi-line statement. Given the choice, I would
> prefer
> > > to keep them.
> > 
> > sounds fine. for what it's worth, the distinction that i've heard is that a
> > multi-line statement requires braces, while a multi-line condition with a
> > single-line statement shouldn't have them:
> > 
> > if (SomeVeryLongConditionIsTrue() &&
> >     SomeOtherVeryLongConditionIsTrue())
> >   DoSomething();
> > 
> > if (ConditionIsTrue) {
> >   DoSomethingWithALotOfArguments(
> >       this_variable_name_seems_awfully_long,
> >       really_cant_you_come_up_with_something_shorter,
> >       it_is_not_like_we_are_paid_by_the_line_after_all);
> > }
> > 
> > but i don't think that this has ever been spelled out in a style guide, and
> i'm
> > not interested in wading through years of style discussions on chromium-dev
to
> > see if it's ever actually come up there. :-P
> 
> It seems that different reviewers interpret the style guide differently. If
you
> are OK with the code as it is, I will just leave it like that rather than
> starting a bikeshed discussion on chromium-dev :).

i'm certainly fine with leaving it like this

[Mattias Nissler (ping if slow), 2013-07-03 16:52:17]

https://codereview.chromium.org/18153007/diff/26002/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/power_policy_browsertest.cc (right):

https://codereview.chromium.org/18153007/diff/26002/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/power_policy_browsertest.cc:66: class
MockPolicyServiceObserver : public PolicyService::Observer {
There's a copy of this in policy_service_impl_unittest.cc already, care to break
it out?

https://codereview.chromium.org/18153007/diff/26002/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/power_policy_browsertest.cc:182:
MockPolicyServiceObserver observer;
Why do we need this observer? The callbacks are certainly not what we're testing
here. Is this code copied from somewhere?

https://codereview.chromium.org/18153007/diff/26002/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/power_policy_browsertest.cc:207:
MockPolicyServiceObserver observer;
Same thing here: Why would we need this observer?

https://codereview.chromium.org/18153007/diff/26002/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/power_policy_browsertest.cc:275:
GetDebugString(power_manager_client_->get_policy()));
Hm, maybe it's simpler to just change all values at once and compare the
strings? Granted, we lose some test coverage (probably nothing critical though),
but the test would be easier to read.

https://codereview.chromium.org/18153007/diff/26002/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/power_policy_browsertest.cc:381:
GetDebugString(power_manager_client_->get_policy()));
Ditto regarding just doing this once and not all policies individually.

https://codereview.chromium.org/18153007/diff/26002/chrome/browser/chromeos/p...
File chrome/browser/chromeos/power/power_prefs.cc (right):

https://codereview.chromium.org/18153007/diff/26002/chrome/browser/chromeos/p...
chrome/browser/chromeos/power/power_prefs.cc:196:
prefs->GetDouble(prefs::kPowerUserActivityScreenDimDelayFactor);
nit: would be nice to add a new line here, I initially failed to see that all
the action is happening in the last line of the function.

https://codereview.chromium.org/18153007/diff/26002/chrome/browser/chromeos/p...
File chrome/browser/chromeos/power/power_prefs_unittest.cc (right):

https://codereview.chromium.org/18153007/diff/26002/chrome/browser/chromeos/p...
chrome/browser/chromeos/power/power_prefs_unittest.cc:10: #include
"base/files/file_path.h"
needed?

https://codereview.chromium.org/18153007/diff/26002/chrome/browser/chromeos/p...
chrome/browser/chromeos/power/power_prefs_unittest.cc:48: const
PrefChangeRegistrar* GetPrefChangeRegistrarUsedByPowerPrefs() const;
Feel free to drop the -UsedByPowerPrefs suffixes unless you do really want them.

https://codereview.chromium.org/18153007/diff/26002/chrome/browser/chromeos/p...
chrome/browser/chromeos/power/power_prefs_unittest.cc:184:
EXPECT_EQ(login_profile->GetPrefs(), pref_change_registrar->prefs());
The PrefChangeRegistrar seems to be an internal implementation detail of the
class, I wouldn't test for this explicitly but rely on the fact that the prefs
must have come from the right profile which you check below.

https://codereview.chromium.org/18153007/diff/26002/chrome/browser/policy/con...
File chrome/browser/policy/configuration_policy_handler.cc (right):

https://codereview.chromium.org/18153007/diff/26002/chrome/browser/policy/con...
chrome/browser/policy/configuration_policy_handler.cc:426: PrefValueMap* prefs)
{
nit: indentation

https://codereview.chromium.org/18153007/diff/26002/chrome/browser/policy/pro...
File chrome/browser/policy/proto/chromeos/chrome_device_policy.proto (right):

https://codereview.chromium.org/18153007/diff/26002/chrome/browser/policy/pro...
chrome/browser/policy/proto/chromeos/chrome_device_policy.proto:416: message
PowerManagementProto {
Ah, so adding this big proto just to remove it shortly after actually does seem
painful to me... anyhow, just go for it.

[bartfab (slow), 2013-07-03 19:11:03]

https://chromiumcodereview.appspot.com/18153007/diff/26002/chrome/browser/chr...
File chrome/browser/chromeos/policy/power_policy_browsertest.cc (right):

https://chromiumcodereview.appspot.com/18153007/diff/26002/chrome/browser/chr...
chrome/browser/chromeos/policy/power_policy_browsertest.cc:66: class
MockPolicyServiceObserver : public PolicyService::Observer {
On 2013/07/03 16:52:18, Mattias Nissler wrote:
> There's a copy of this in policy_service_impl_unittest.cc already, care to
break
> it out?

Done.

https://chromiumcodereview.appspot.com/18153007/diff/26002/chrome/browser/chr...
chrome/browser/chromeos/policy/power_policy_browsertest.cc:182:
MockPolicyServiceObserver observer;
On 2013/07/03 16:52:18, Mattias Nissler wrote:
> Why do we need this observer? The callbacks are certainly not what we're
testing
> here. Is this code copied from somewhere?

We call policy_manager->core()->store()->Load() and then have to wait for the
policy to be loaded, verified and applied (which happens in the asynchronously
in the blocking pool). I followed the approach I saw in another test: Set up an
observer that will call base::RunLoop::Quit() when the policy arrives, then
simply base::RunLoop()::Run().

https://chromiumcodereview.appspot.com/18153007/diff/26002/chrome/browser/chr...
chrome/browser/chromeos/policy/power_policy_browsertest.cc:207:
MockPolicyServiceObserver observer;
On 2013/07/03 16:52:18, Mattias Nissler wrote:
> Same thing here: Why would we need this observer?

As above.

https://chromiumcodereview.appspot.com/18153007/diff/26002/chrome/browser/chr...
chrome/browser/chromeos/policy/power_policy_browsertest.cc:275:
GetDebugString(power_manager_client_->get_policy()));
On 2013/07/03 16:52:18, Mattias Nissler wrote:
> Hm, maybe it's simpler to just change all values at once and compare the
> strings? Granted, we lose some test coverage (probably nothing critical
though),
> but the test would be easier to read.

Done.

https://chromiumcodereview.appspot.com/18153007/diff/26002/chrome/browser/chr...
chrome/browser/chromeos/policy/power_policy_browsertest.cc:381:
GetDebugString(power_manager_client_->get_policy()));
On 2013/07/03 16:52:18, Mattias Nissler wrote:
> Ditto regarding just doing this once and not all policies individually.

Done.

https://chromiumcodereview.appspot.com/18153007/diff/26002/chrome/browser/chr...
File chrome/browser/chromeos/power/power_prefs.cc (right):

https://chromiumcodereview.appspot.com/18153007/diff/26002/chrome/browser/chr...
chrome/browser/chromeos/power/power_prefs.cc:196:
prefs->GetDouble(prefs::kPowerUserActivityScreenDimDelayFactor);
On 2013/07/03 16:52:18, Mattias Nissler wrote:
> nit: would be nice to add a new line here, I initially failed to see that all
> the action is happening in the last line of the function.

Done.

https://chromiumcodereview.appspot.com/18153007/diff/26002/chrome/browser/chr...
File chrome/browser/chromeos/power/power_prefs_unittest.cc (right):

https://chromiumcodereview.appspot.com/18153007/diff/26002/chrome/browser/chr...
chrome/browser/chromeos/power/power_prefs_unittest.cc:10: #include
"base/files/file_path.h"
On 2013/07/03 16:52:18, Mattias Nissler wrote:
> needed?

If I strictly follow IWYU, yes:

profile_manager_.profiles_dir() returns a base::FilePath& and I pass the value
on to another method.

https://chromiumcodereview.appspot.com/18153007/diff/26002/chrome/browser/chr...
chrome/browser/chromeos/power/power_prefs_unittest.cc:48: const
PrefChangeRegistrar* GetPrefChangeRegistrarUsedByPowerPrefs() const;
On 2013/07/03 16:52:18, Mattias Nissler wrote:
> Feel free to drop the -UsedByPowerPrefs suffixes unless you do really want
them.

Done.

https://chromiumcodereview.appspot.com/18153007/diff/26002/chrome/browser/chr...
chrome/browser/chromeos/power/power_prefs_unittest.cc:184:
EXPECT_EQ(login_profile->GetPrefs(), pref_change_registrar->prefs());
On 2013/07/03 16:52:18, Mattias Nissler wrote:
> The PrefChangeRegistrar seems to be an internal implementation detail of the
> class, I wouldn't test for this explicitly but rely on the fact that the prefs
> must have come from the right profile which you check below.

Done.

https://chromiumcodereview.appspot.com/18153007/diff/26002/chrome/browser/pol...
File chrome/browser/policy/configuration_policy_handler.cc (right):

https://chromiumcodereview.appspot.com/18153007/diff/26002/chrome/browser/pol...
chrome/browser/policy/configuration_policy_handler.cc:426: PrefValueMap* prefs)
{
On 2013/07/03 16:52:18, Mattias Nissler wrote:
> nit: indentation

Done.

https://chromiumcodereview.appspot.com/18153007/diff/26002/chrome/browser/pol...
File chrome/browser/policy/proto/chromeos/chrome_device_policy.proto (right):

https://chromiumcodereview.appspot.com/18153007/diff/26002/chrome/browser/pol...
chrome/browser/policy/proto/chromeos/chrome_device_policy.proto:416: message
PowerManagementProto {
On 2013/07/03 16:52:18, Mattias Nissler wrote:
> Ah, so adding this big proto just to remove it shortly after actually does
seem
> painful to me... anyhow, just go for it.

I will see how much effort it is to add a ConfigurationPolicyHandler that
verifies a JSON schema. Maybe I can land such a handler class first and then
land this CL with a single JSON policy.

[Mattias Nissler (ping if slow), 2013-07-04 11:53:04]

LGTM w/ nits and after addressing the RefreshPolicies comment.

https://codereview.chromium.org/18153007/diff/26002/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/power_policy_browsertest.cc (right):

https://codereview.chromium.org/18153007/diff/26002/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/power_policy_browsertest.cc:182:
MockPolicyServiceObserver observer;
On 2013/07/03 19:11:04, bartfab wrote:
> On 2013/07/03 16:52:18, Mattias Nissler wrote:
> > Why do we need this observer? The callbacks are certainly not what we're
> testing
> > here. Is this code copied from somewhere?
> 
> We call policy_manager->core()->store()->Load() and then have to wait for the
> policy to be loaded, verified and applied (which happens in the asynchronously
> in the blocking pool). I followed the approach I saw in another test: Set up
an
> observer that will call base::RunLoop::Quit() when the policy arrives, then
> simply base::RunLoop()::Run().

Ah, I had missed the waiting part. I guess a better way of doing this would be
to call PolicyService::RefreshPolicies(), which takes a completion callback
parameter (for which you can pass run_loop.QuitClosure()).

https://codereview.chromium.org/18153007/diff/38001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/power_policy_browsertest.cc (right):

https://codereview.chromium.org/18153007/diff/38001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/power_policy_browsertest.cc:111:
MockPolicyServiceObserver::MockPolicyServiceObserver() {
These shouldn't be here?

https://codereview.chromium.org/18153007/diff/38001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/power/power_prefs_unittest.cc (right):

https://codereview.chromium.org/18153007/diff/38001/chrome/browser/chromeos/p...
chrome/browser/chromeos/power/power_prefs_unittest.cc:203: 
nit: remove extra blank line.

[bartfab (slow), 2013-07-05 13:52:57]

https://codereview.chromium.org/18153007/diff/26002/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/power_policy_browsertest.cc (right):

https://codereview.chromium.org/18153007/diff/26002/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/power_policy_browsertest.cc:182:
MockPolicyServiceObserver observer;
On 2013/07/04 11:53:04, Mattias Nissler wrote:
> On 2013/07/03 19:11:04, bartfab wrote:
> > On 2013/07/03 16:52:18, Mattias Nissler wrote:
> > > Why do we need this observer? The callbacks are certainly not what we're
> > testing
> > > here. Is this code copied from somewhere?
> > 
> > We call policy_manager->core()->store()->Load() and then have to wait for
the
> > policy to be loaded, verified and applied (which happens in the
asynchronously
> > in the blocking pool). I followed the approach I saw in another test: Set up
> an
> > observer that will call base::RunLoop::Quit() when the policy arrives, then
> > simply base::RunLoop()::Run().
> 
> Ah, I had missed the waiting part. I guess a better way of doing this would be
> to call PolicyService::RefreshPolicies(), which takes a completion callback
> parameter (for which you can pass run_loop.QuitClosure()). 

As discussed offline, I kept this implementation to avoid the need to fake
registration against DMServer but I refactored things to factor out common
boilerplate code.

https://codereview.chromium.org/18153007/diff/38001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/power_policy_browsertest.cc (right):

https://codereview.chromium.org/18153007/diff/38001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/power_policy_browsertest.cc:111:
MockPolicyServiceObserver::MockPolicyServiceObserver() {
On 2013/07/04 11:53:04, Mattias Nissler wrote:
> These shouldn't be here?

Done.

https://codereview.chromium.org/18153007/diff/38001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/power/power_prefs_unittest.cc (right):

https://codereview.chromium.org/18153007/diff/38001/chrome/browser/chromeos/p...
chrome/browser/chromeos/power/power_prefs_unittest.cc:203: 
On 2013/07/04 11:53:04, Mattias Nissler wrote:
> nit: remove extra blank line.

Done.

[Mattias Nissler (ping if slow), 2013-07-05 14:26:05]

LGTM

[bartfab (slow), 2013-07-08 14:04:24]

Hi Mattias,

Could you take another look? I replaced the 10 new policies with a single new
JSON policy.

[Mattias Nissler (ping if slow), 2013-07-08 17:36:57]

Reading the code, I felt that we'd be better off if we had just a single pref
holding the dictionary. Would you agree?

If so, what would be your suggestion to get there eventually? Convert both
session prefs and login screen prefs in one go eventually? Introduce the
dictionary-value pref right now and only use it for login screen prefs? Other
ideas?

https://codereview.chromium.org/18153007/diff/79001/chrome/app/policy/policy_...
File chrome/app/policy/policy_templates.json (right):

https://codereview.chromium.org/18153007/diff/79001/chrome/app/policy/policy_...
chrome/app/policy/policy_templates.json:4509: This policy lets you configure how
<ph name="PRODUCT_OS_NAME">$2<ex>Google Chrome OS</ex></ph> behaves when there
is no user activity for some amount of time while the login screen is being
shown. The policy controls multiple settings. For their individual semantics and
value ranges, see the corresponding policies that control power management
within a session. The only deviations from these policies are:
Are we planning on redoing the session power policies to be a dictionary as
well? If so, you'll have to write a full description eventually ;)

https://codereview.chromium.org/18153007/diff/79001/chrome/app/policy/policy_...
chrome/app/policy/policy_templates.json:4513: The policy should be specified as
a string that expresses the individual settings in JSON format, conforming to
the following schema:
Did you check with Joao what the story is on dictionary-valued policy? I assume
the plan is to go with string for now and switch over to a proper schema here
once end-to-end support for dictionary types becomes available?

It might be helpful to add a TODO here providing some context.

https://codereview.chromium.org/18153007/diff/79001/chrome/app/policy/policy_...
chrome/app/policy/policy_templates.json:4538: "enum": [ 0, 2, 3 ]
This seems unfortunate. Can we do a string enum instead?

https://codereview.chromium.org/18153007/diff/79001/chrome/app/policy/policy_...
chrome/app/policy/policy_templates.json:4563: "enum": [ 0, 2, 3 ]
ditto

https://codereview.chromium.org/18153007/diff/79001/chrome/app/policy/policy_...
chrome/app/policy/policy_templates.json:4568: "enum": [ 0, 2, 3 ]
ditto

https://codereview.chromium.org/18153007/diff/79001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc (right):

https://codereview.chromium.org/18153007/diff/79001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc:588:
Value::CreateStringValue(
It'd be nice to pass a dictionary value here if possible.

https://codereview.chromium.org/18153007/diff/79001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/login_profile_policy_provider.cc (right):

https://codereview.chromium.org/18153007/diff/79001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/login_profile_policy_provider.cc:137:
ApplyValueAsMandatoryPolicy(power_management_policy.GetScreenDimDelayAC(),
I guess we eventually want to switch to a dictionary-valued pref as well, would
you agree?

We can do that when converting the profile pref to that format as well. File a
bug and put a TODO here?

https://codereview.chromium.org/18153007/diff/79001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/login_screen_power_management_policy.h
(right):

https://codereview.chromium.org/18153007/diff/79001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/login_screen_power_management_policy.h:20: 
Class comment: What does it do, what is it good for.

[bartfab (slow), 2013-07-09 10:02:33]

Hi Mattias,

Please take another look.

https://codereview.chromium.org/18153007/diff/79001/chrome/app/policy/policy_...
File chrome/app/policy/policy_templates.json (right):

https://codereview.chromium.org/18153007/diff/79001/chrome/app/policy/policy_...
chrome/app/policy/policy_templates.json:4509: This policy lets you configure how
<ph name="PRODUCT_OS_NAME">$2<ex>Google Chrome OS</ex></ph> behaves when there
is no user activity for some amount of time while the login screen is being
shown. The policy controls multiple settings. For their individual semantics and
value ranges, see the corresponding policies that control power management
within a session. The only deviations from these policies are:
On 2013/07/08 17:36:57, Mattias Nissler wrote:
> Are we planning on redoing the session power policies to be a dictionary as
> well? If so, you'll have to write a full description eventually ;)

Yes, at that point, I will just consolidate the descriptions of the individual
policies into a single description. For now, we need to keep the individual
policies documented. I do not think that duplicating the documentation here
would be very useful.

https://codereview.chromium.org/18153007/diff/79001/chrome/app/policy/policy_...
chrome/app/policy/policy_templates.json:4513: The policy should be specified as
a string that expresses the individual settings in JSON format, conforming to
the following schema:
On 2013/07/08 17:36:57, Mattias Nissler wrote:
> Did you check with Joao what the story is on dictionary-valued policy? I
assume
> the plan is to go with string for now and switch over to a proper schema here
> once end-to-end support for dictionary types becomes available?
> 
> It might be helpful to add a TODO here providing some context.

I added a TODO. And yes, I had checked with Joao. His recommendation is to go
with 'string' for now. Once 'dict' is fully supported, it will be trivial to
switch to that as the cloud wire protocol will remain the same - a 'dict' will
still be expressed as a JSON string in the protobuf.

https://codereview.chromium.org/18153007/diff/79001/chrome/app/policy/policy_...
chrome/app/policy/policy_templates.json:4538: "enum": [ 0, 2, 3 ]
On 2013/07/08 17:36:57, Mattias Nissler wrote:
> This seems unfortunate. Can we do a string enum instead?

Done.

https://codereview.chromium.org/18153007/diff/79001/chrome/app/policy/policy_...
chrome/app/policy/policy_templates.json:4563: "enum": [ 0, 2, 3 ]
On 2013/07/08 17:36:57, Mattias Nissler wrote:
> ditto

Done.

https://codereview.chromium.org/18153007/diff/79001/chrome/app/policy/policy_...
chrome/app/policy/policy_templates.json:4568: "enum": [ 0, 2, 3 ]
On 2013/07/08 17:36:57, Mattias Nissler wrote:
> ditto

Done.

https://codereview.chromium.org/18153007/diff/79001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc (right):

https://codereview.chromium.org/18153007/diff/79001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc:588:
Value::CreateStringValue(
On 2013/07/08 17:36:57, Mattias Nissler wrote:
> It'd be nice to pass a dictionary value here if possible.

Right now, string is the better choice. To get a dict, you have to parse the
JSON string from the protobuf. If you try parsing here and the string is
invalid, you get no policy at all and no error message in about:policy to tell
you what went wrong. If it remains a string here, the parsing happens later and
any parsing errors show up in about:policy.

There is also a second reason to leave it as a string: The 'dict' user policy
type is actually aliased to a string for now and only gets parsed in the
ConfigurationPolicyHandler. Since the JSON power management device policy is
meant to be the first step toward JSON power management policy throughout, it
should be consistent with what a 'dict' JSON power management user policy will
look like. If both are expressed as strings, the same code can be used to parse
them. Only once the 'dict' user policy type switches to actually being a
DictionaryValue should the device policy switch as well.

https://codereview.chromium.org/18153007/diff/79001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/login_profile_policy_provider.cc (right):

https://codereview.chromium.org/18153007/diff/79001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/login_profile_policy_provider.cc:137:
ApplyValueAsMandatoryPolicy(power_management_policy.GetScreenDimDelayAC(),
On 2013/07/08 17:36:57, Mattias Nissler wrote:
> I guess we eventually want to switch to a dictionary-valued pref as well,
would
> you agree?
> 
> We can do that when converting the profile pref to that format as well. File a
> bug and put a TODO here?

Done. For reference, making that additional switch now would be a bad idea: The
'dict' policy type is still aliased to a string for now. This means that the
device policy would need to be expressed as a string-valued user policy. Since
some power management settings are allowed in user policy but not in device
policy, the work flow would be as follows:

* Decode device policy from JSON string to DictionaryValue
* Remove any power management settings not allowed on the login screen
* Re-encode the settings from Dictionary Value to JSON string
* Set the string as user policy for the login profile
* Decode user policy from JSON string to DictionaryValue

We would end up decoding, encoding, decoding JSON - hardly elegant or efficient.
Once both user and device policy are actually dict-valued, we will be able to do
this much more smoothly.

https://codereview.chromium.org/18153007/diff/79001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/login_screen_power_management_policy.h
(right):

https://codereview.chromium.org/18153007/diff/79001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/login_screen_power_management_policy.h:20: 
On 2013/07/08 17:36:57, Mattias Nissler wrote:
> Class comment: What does it do, what is it good for.

Done.

[Mattias Nissler (ping if slow), 2013-07-10 09:45:13]

LGTM w/ nits.

https://codereview.chromium.org/18153007/diff/88001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/login_screen_power_management_policy.cc
(right):

https://codereview.chromium.org/18153007/diff/88001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/login_screen_power_management_policy.cc:19: const
char kScreenDimDelayAC[]      = "AC.Delays.ScreenDim";
nit: no identation.

https://codereview.chromium.org/18153007/diff/88001/chrome/browser/chromeos/p...
File
chrome/browser/chromeos/policy/login_screen_power_management_policy_unittest.cc
(right):

https://codereview.chromium.org/18153007/diff/88001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/login_screen_power_management_policy_unittest.cc:68:
errors.Clear();
nit: can we put newlines between the blocks to make clear which code checks a
single setting?

https://codereview.chromium.org/18153007/diff/88001/chrome/browser/policy/pro...
File chrome/browser/policy/proto/chromeos/chrome_device_policy.proto (right):

https://codereview.chromium.org/18153007/diff/88001/chrome/browser/policy/pro...
chrome/browser/policy/proto/chromeos/chrome_device_policy.proto:419: // format,
conforming to the following schema:
We'll likely need descriptions for the individual settings, and the server
people will rightfully ask for them. Can you add "description" fields to the
schema please?

[bartfab (slow), 2013-07-16 11:07:34]

https://chromiumcodereview.appspot.com/18153007/diff/88001/chrome/browser/chr...
File chrome/browser/chromeos/policy/login_screen_power_management_policy.cc
(right):

https://chromiumcodereview.appspot.com/18153007/diff/88001/chrome/browser/chr...
chrome/browser/chromeos/policy/login_screen_power_management_policy.cc:19: const
char kScreenDimDelayAC[]      = "AC.Delays.ScreenDim";
On 2013/07/10 09:45:13, Mattias Nissler wrote:
> nit: no identation.

Done.

https://chromiumcodereview.appspot.com/18153007/diff/88001/chrome/browser/chr...
File
chrome/browser/chromeos/policy/login_screen_power_management_policy_unittest.cc
(right):

https://chromiumcodereview.appspot.com/18153007/diff/88001/chrome/browser/chr...
chrome/browser/chromeos/policy/login_screen_power_management_policy_unittest.cc:68:
errors.Clear();
On 2013/07/10 09:45:13, Mattias Nissler wrote:
> nit: can we put newlines between the blocks to make clear which code checks a
> single setting?

Done.

https://chromiumcodereview.appspot.com/18153007/diff/88001/chrome/browser/pol...
File chrome/browser/policy/proto/chromeos/chrome_device_policy.proto (right):

https://chromiumcodereview.appspot.com/18153007/diff/88001/chrome/browser/pol...
chrome/browser/policy/proto/chromeos/chrome_device_policy.proto:419: // format,
conforming to the following schema:
On 2013/07/10 09:45:13, Mattias Nissler wrote:
> We'll likely need descriptions for the individual settings, and the server
> people will rightfully ask for them. Can you add "description" fields to the
> schema please?

Thanks for pointing out the "description" field. I had been looking for some way
to add comments to the schema and I missed exactly that field in the JSON schema
spec.

Done.

[commit-bot: I haz the power, 2013-07-16 11:10:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bartfab@chromium.org/18153007/128002

[commit-bot: I haz the power, 2013-07-16 12:59:53]

Retried try job too often on linux_aura for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-07-16 13:06:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bartfab@chromium.org/18153007/128002

[commit-bot: I haz the power, 2013-07-16 14:52:36]

Retried try job too often on linux_aura for step(s) content_browsertests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-07-16 14:59:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bartfab@chromium.org/18153007/128002

[commit-bot: I haz the power, 2013-07-16 15:34:36]

Retried try job too often on linux_aura for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-07-16 21:22:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bartfab@chromium.org/18153007/128002

[commit-bot: I haz the power, 2013-07-17 00:53:31]

Retried try job too often on win7_aura for step(s) content_browsertests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-07-17 08:10:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bartfab@chromium.org/18153007/128002

[commit-bot: I haz the power, 2013-07-17 13:44:45]

Change committed as 212024