chromeos: Move default power management policy into Chrome.

chromeos: Move default power management policy into Chrome.

This updates PowerPolicyController to always send the power
management policy contained in Chrome's preferences to the
power manager, rather than only sending prefs that have been
changed from their defaults.  Note that this currently only
takes effect once the user has logged in.

It also makes the screen get locked at the same time as when
it's turned off when the "Require password to wake from
sleep" setting is enabled.

BUG=225730, 225969
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=195376

[bartfab (slow), 2013-04-17 11:51:12]

https://codereview.chromium.org/14134004/diff/3007/chromeos/dbus/power_policy...
File chromeos/dbus/power_policy_controller.cc (right):

https://codereview.chromium.org/14134004/diff/3007/chromeos/dbus/power_policy...
chromeos/dbus/power_policy_controller.cc:145: if
(enable_screen_lock_pref_.GetValue() && delays->screen_off_ms() > 0 &&
Nit: Add a comment that explains this.

https://codereview.chromium.org/14134004/diff/3007/chromeos/dbus/power_policy...
chromeos/dbus/power_policy_controller.cc:157: if
(enable_screen_lock_pref_.GetValue() && delays->screen_off_ms() > 0 &&
Nit: Add a comment that explains this.

https://codereview.chromium.org/14134004/diff/3007/chromeos/dbus/power_policy...
File chromeos/dbus/power_policy_controller.h (right):

https://codereview.chromium.org/14134004/diff/3007/chromeos/dbus/power_policy...
chromeos/dbus/power_policy_controller.h:14: #include "base/prefs/pref_service.h"
Nit: It would be sufficient to forward-declare PolicyService here and do the
include in the implementation file.

https://codereview.chromium.org/14134004/diff/3007/chromeos/dbus/power_policy...
chromeos/dbus/power_policy_controller.h:100: PrefNames pref_names_;
I don't think managing the PrefMembers in this class is a good idea. Passing
pref names from chrome/ to chromeos/dbus/ seems awkward. Also, the PrefNames 
struct ends up occupying a good chunk of memory to store copies of pref names.

https://codereview.chromium.org/14134004/diff/3007/chromeos/dbus/power_policy...
chromeos/dbus/power_policy_controller.h:129:
base::WeakPtrFactory<PowerPolicyController> weak_ptr_factory_;
Nit: Move the #include "base/memory/weak_ptr.h" from the implementation file to
the header file.

https://codereview.chromium.org/14134004/diff/3007/chromeos/dbus/power_policy...
File chromeos/dbus/power_policy_controller_unittest.cc (right):

https://codereview.chromium.org/14134004/diff/3007/chromeos/dbus/power_policy...
chromeos/dbus/power_policy_controller_unittest.cc:79:
names.ac_screen_lock_delay_ms =kAcScreenLockDelay;
Nit: Space after =

[Daniel Erat, 2013-04-17 13:52:13]

https://codereview.chromium.org/14134004/diff/3007/chromeos/dbus/power_policy...
File chromeos/dbus/power_policy_controller.h (right):

https://codereview.chromium.org/14134004/diff/3007/chromeos/dbus/power_policy...
chromeos/dbus/power_policy_controller.h:100: PrefNames pref_names_;
On 2013/04/17 11:51:12, bartfab wrote:
> I don't think managing the PrefMembers in this class is a good idea. Passing
> pref names from chrome/ to chromeos/dbus/ seems awkward. Also, the PrefNames 
> struct ends up occupying a good chunk of memory to store copies of pref names.

What's your preference for how this should work instead?  Should the PrefMembers
remain in chromeos::Preferences, this struct instead contain pref values (e.g.
int, bool, etc.), and it get passed to a HandleUpdatedPreferences() method?

[Daniel Erat, 2013-04-18 03:59:15]

PTAL

https://codereview.chromium.org/14134004/diff/3007/chromeos/dbus/power_policy...
File chromeos/dbus/power_policy_controller.cc (right):

https://codereview.chromium.org/14134004/diff/3007/chromeos/dbus/power_policy...
chromeos/dbus/power_policy_controller.cc:145: if
(enable_screen_lock_pref_.GetValue() && delays->screen_off_ms() > 0 &&
On 2013/04/17 11:51:12, bartfab wrote:
> Nit: Add a comment that explains this.

Done.

https://codereview.chromium.org/14134004/diff/3007/chromeos/dbus/power_policy...
chromeos/dbus/power_policy_controller.cc:157: if
(enable_screen_lock_pref_.GetValue() && delays->screen_off_ms() > 0 &&
On 2013/04/17 11:51:12, bartfab wrote:
> Nit: Add a comment that explains this.

skipped this since i added the earlier comment

https://codereview.chromium.org/14134004/diff/3007/chromeos/dbus/power_policy...
File chromeos/dbus/power_policy_controller.h (right):

https://codereview.chromium.org/14134004/diff/3007/chromeos/dbus/power_policy...
chromeos/dbus/power_policy_controller.h:14: #include "base/prefs/pref_service.h"
On 2013/04/17 11:51:12, bartfab wrote:
> Nit: It would be sufficient to forward-declare PolicyService here and do the
> include in the implementation file.

n/a

https://codereview.chromium.org/14134004/diff/3007/chromeos/dbus/power_policy...
chromeos/dbus/power_policy_controller.h:129:
base::WeakPtrFactory<PowerPolicyController> weak_ptr_factory_;
On 2013/04/17 11:51:12, bartfab wrote:
> Nit: Move the #include "base/memory/weak_ptr.h" from the implementation file
to
> the header file.

n/a

https://codereview.chromium.org/14134004/diff/3007/chromeos/dbus/power_policy...
File chromeos/dbus/power_policy_controller_unittest.cc (right):

https://codereview.chromium.org/14134004/diff/3007/chromeos/dbus/power_policy...
chromeos/dbus/power_policy_controller_unittest.cc:79:
names.ac_screen_lock_delay_ms =kAcScreenLockDelay;
On 2013/04/17 11:51:12, bartfab wrote:
> Nit: Space after =

n/a

[bartfab (slow), 2013-04-18 14:17:22]

Thanks for moving the pref management back to chrome. I think this is a lot
better.

LGTM with two small nits.

https://codereview.chromium.org/14134004/diff/12001/chromeos/dbus/power_polic...
File chromeos/dbus/power_policy_controller.cc (right):

https://codereview.chromium.org/14134004/diff/12001/chromeos/dbus/power_polic...
chromeos/dbus/power_policy_controller.cc:8: #include "base/values.h"
Nit: No longer used.

https://codereview.chromium.org/14134004/diff/12001/chromeos/dbus/power_polic...
File chromeos/dbus/power_policy_controller.h (right):

https://codereview.chromium.org/14134004/diff/12001/chromeos/dbus/power_polic...
chromeos/dbus/power_policy_controller.h:39: : ac_screen_dim_delay_ms(-1),
As discussed offline, please add a comment that explains the rationale behind
these values.

[Daniel Erat, 2013-04-18 14:37:10]

https://codereview.chromium.org/14134004/diff/12001/chromeos/dbus/power_polic...
File chromeos/dbus/power_policy_controller.cc (right):

https://codereview.chromium.org/14134004/diff/12001/chromeos/dbus/power_polic...
chromeos/dbus/power_policy_controller.cc:8: #include "base/values.h"
On 2013/04/18 14:17:22, bartfab wrote:
> Nit: No longer used.

Done.

https://codereview.chromium.org/14134004/diff/12001/chromeos/dbus/power_polic...
File chromeos/dbus/power_policy_controller.h (right):

https://codereview.chromium.org/14134004/diff/12001/chromeos/dbus/power_polic...
chromeos/dbus/power_policy_controller.h:39: : ac_screen_dim_delay_ms(-1),
On 2013/04/18 14:17:22, bartfab wrote:
> As discussed offline, please add a comment that explains the rationale behind
> these values.

Done.

[commit-bot: I haz the power, 2013-04-18 18:37:02]

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

[commit-bot: I haz the power, 2013-04-18 19:09:00]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-04-18 19:13:08]

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

[Daniel Erat, 2013-04-18 21:09:40]

I'm having a miserable time trying to update PowerPolicyBrowserTest for this
change.  It makes the assumption that the only fields that will be set in
PowerManagementPolicy messages are the ones corresponding to the user policies
that it set, which is no longer the case after this change.

To some degree, it feels like the test is overreaching by looking at
PowerManagementPolicy messages -- shouldn't it just be checking that the prefs
are updated correctly?

I'm pretty much stuck with regard to how to fix it.  I tried saving the
PowerManagementPolicy that gets sent just before the user policy gets changed
and using that as a baseline, but I'm still getting failures that I don't
understand.  My eyes are glazing over from looking at error messages like the
following.  Needless to say, I'm not a fan of gMock. :-(

unknown file: Failure
            
Unexpected mock function call - returning directly.
    Function call: SetPolicy(@0x7fff6c83bfc0 64-byte object <B0-9E 84-F7 06-7F
00-00 00-00 00-00 00-00 00-00 80-8B 4C-2B B1-17 00-00 20-1E EF-2B B1-17 00-00
00-00 00-00 00-00 00-40 A0-DE 32-2A B1-17 00-00 01-01 6F-F7 00-00 00-00 FF-00
00-00 00-00 00-00>)
Google Mock tried the following 1 expectation, but it didn't match:
            
../../chrome/browser/chromeos/policy/power_policy_browsertest.cc:108:
EXPECT_CALL(*power_manager_client_,
SetPolicy(PowerManagementPolicyMatches(power_management_policy)))...
  Expected arg #0: does not match the expected power management policy
           Actual: 64-byte object <B0-9E 84-F7 06-7F 00-00 00-00 00-00 00-00
00-00 80-8B 4C-2B B1-17 00-00 20-1E EF-2B B1-17 00-00 00-00 00-00 00-00 00-40
A0-DE 32-2A B1-17 00-00 01-01 6F-F7 00-00 00-00 FF-00 00-00 00-00 00-00>
         Expected: to be called at least once
           Actual: never called - unsatisfied and active
unknown file: Failure
            
Unexpected mock function call - returning directly.
    Function call: SetPolicy(@0x7fff6c83bfc0 64-byte object <B0-9E 84-F7 06-7F
00-00 00-00 00-00 01-00 00-00 B0-26 2E-2B B1-17 00-00 20-A0 4A-2B B1-17 00-00
00-00 00-00 00-00 00-40 20-D6 6F-2A B1-17 00-00 01-01 6F-F7 00-00 00-00 FF-00
00-00 00-00 00-00>)
Google Mock tried the following 1 expectation, but it didn't match:
            
../../chrome/browser/chromeos/policy/power_policy_browsertest.cc:108:
EXPECT_CALL(*power_manager_client_,
SetPolicy(PowerManagementPolicyMatches(power_management_policy)))...
  Expected arg #0: does not match the expected power management policy
           Actual: 64-byte object <B0-9E 84-F7 06-7F 00-00 00-00 00-00 01-00
00-00 B0-26 2E-2B B1-17 00-00 20-A0 4A-2B B1-17 00-00 00-00 00-00 00-00 00-40
20-D6 6F-2A B1-17 00-00 01-01 6F-F7 00-00 00-00 FF-00 00-00 00-00 00-00>
         Expected: to be called at least once
           Actual: never called - unsatisfied and active
../../chrome/browser/chromeos/policy/power_policy_browsertest.cc:108: Failure
Actual function call count doesn't match EXPECT_CALL(*power_manager_client_,
SetPolicy(PowerManagementPolicyMatches(power_management_policy)))...
         Expected: to be called at least once
           Actual: never called - unsatisfied and active

[commit-bot: I haz the power, 2013-04-18 21:15:17]

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

[Daniel Erat, 2013-04-19 14:22:36]

PTAL

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

https://codereview.chromium.org/14134004/diff/44001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/power_policy_browsertest.cc:125:
power_management_policy.mutable_ac_delays()->set_idle_ms(9000);
Changing these to different values for each delay and to shorter values so some
lines can be unwrapped.

[bartfab (slow), 2013-04-19 16:28:48]

lgtm

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

https://codereview.chromium.org/14134004/diff/51002/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/power_policy_browsertest.cc:73:
.WillRepeatedly(Return());
Nit: It should be sufficient to say .Times(AnyNumber()). A simple return is the
default action anyway.

[Daniel Erat, 2013-04-20 01:46:21]

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

https://codereview.chromium.org/14134004/diff/51002/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/power_policy_browsertest.cc:73:
.WillRepeatedly(Return());
On 2013/04/19 16:28:48, bartfab wrote:
> Nit: It should be sufficient to say .Times(AnyNumber()). A simple return is
the
> default action anyway.

Done.

[commit-bot: I haz the power, 2013-04-20 01:46:56]

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

[commit-bot: I haz the power, 2013-04-20 02:06:38]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-04-20 02:08:41]

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

[commit-bot: I haz the power, 2013-04-20 02:14:10]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win7_aura.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-04-20 02:35:28]

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

[commit-bot: I haz the power, 2013-04-20 02:45:16]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-04-20 03:15:24]

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

[commit-bot: I haz the power, 2013-04-20 03:21:12]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_x64_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[Daniel Erat, 2013-04-20 04:03:02]

Committed patchset #9 manually as r195376 (presubmit successful).