Add device policies to control accessibility settings on the login screen

Add device policies to control accessibility settings on the login screen

This CL adds device policies that provide control over the default state
of the following four accessibility settings on the login screen:
* Large cursor
* Spoken feedback
* High contrast mode
* Screen magnifier type

The User can temporarily override the settings but the defaults are
restored whenever the login screen is shown anew or the user remains idle
on the login screen for one minute.

BUG=225955, 225956, 243350, 247298
TEST=New unittests + new browsertests + manual in VM

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

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

[bartfab (slow), 2013-06-11 23:26:24]

Hi Mattias,

Could you review the policy part of this CL (essentially, all of it)?

Hi Sailesh,

Could you do an owner review of:
chrome_browser_main_extra_parts_profiles.cc

Hi Scott,

Could you do an owner review of:
chrome/test/base/testing_profile_manager.[h|cc]

[sky, 2013-06-11 23:33:37]

testing_profile_manager LGTM

[sail, 2013-06-12 00:05:17]

profiles/* LGTM

[Mattias Nissler (ping if slow), 2013-06-12 13:55:16]

I think the general approach with the new policy provider is rather neat.

Some hiccups as pointed out in comments. One thing I don't fully grok is why you
need these TestingProfile changes?

https://codereview.chromium.org/16658015/diff/8001/chrome/app/policy/policy_t...
File chrome/app/policy/policy_templates.json (right):

https://codereview.chromium.org/16658015/diff/8001/chrome/app/policy/policy_t...
chrome/app/policy/policy_templates.json:4608: 'name':
'LoginScreenDefaultLargeCursorEnabled',
We've named other policy DeviceXYZ, maybe do the same here for consistency?

https://codereview.chromium.org/16658015/diff/8001/chrome/browser/chromeos/po...
File chrome/browser/chromeos/policy/device_policy_browsertest.cc (right):

https://codereview.chromium.org/16658015/diff/8001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/device_policy_browsertest.cc:88: class
DevicePolicyBrowsertest : public DevicePolicyCrosBrowserTest,
The name of this class and file is a bit misleading. Should probably be
DevicePolicyDefaultsBrowserTest?

https://codereview.chromium.org/16658015/diff/8001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/device_policy_browsertest.cc:114: if (logged_in_)
It seems like logged_in_ behavior and test verification is vastly different from
!logged_in_ behavior. Does it really make sense to treat them the same?

https://codereview.chromium.org/16658015/diff/8001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/device_policy_browsertest.cc:145:
install_attrs_file));
Given this is a copy of existing code, we should probably make it available via
DevicePolicyCrosBrowserTest.

https://codereview.chromium.org/16658015/diff/8001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/device_policy_browsertest.cc:197: Profile*
current_profile = logged_in_ ? ProfileManager::GetDefaultProfile() :
style nit: not sure, but I think the colon commonly goes on the next line,
aligned with the ?

https://codereview.chromium.org/16658015/diff/8001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/device_policy_browsertest.cc:317: }
The previous 3 test cases are identical in terms of code (except for the pref
name and the last line) AFAICS. Can they share code?

https://codereview.chromium.org/16658015/diff/8001/chrome/browser/chromeos/po...
File chrome/browser/chromeos/policy/login_profile_policy_provider.cc (right):

https://codereview.chromium.org/16658015/diff/8001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/login_profile_policy_provider.cc:64: return
device_policy_service_->IsInitializationComplete(domain);
This might hold startup, have you checked?

https://codereview.chromium.org/16658015/diff/8001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/login_profile_policy_provider.cc:71:
weak_factory_.GetWeakPtr()));
The contract is that if you receive a RefreshPolicies() call, the next
UpdatePolicy() needs to contain the changes pulled in via that. So you should
probably check weak_factory_.HasWeakPtrs() in UpdateFromDevicePolicy() and only
perform the update once the actual callback runs. You'll also want to do a
weak_factory_.InvalidateWeakPtrs() here.

https://codereview.chromium.org/16658015/diff/8001/chrome/browser/chromeos/po...
File chrome/browser/chromeos/policy/recommendation_restorer.cc (right):

https://codereview.chromium.org/16658015/diff/8001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/recommendation_restorer.cc:89: if
(!base::Value::Equals(recommended_value, recommended_value_entry)) {
So this recommended_values_ thing is just here to detect whether the pref change
was user-triggered, instead of admin-triggered?

Can't you just ignore pref changes entirely here and just call ClearPref() on
all prefs when the timer expires? I don't see what would be wrong with that.

https://codereview.chromium.org/16658015/diff/8001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/recommendation_restorer.cc:139:
chromeos::switches::kLoginManager)) {
So IsIsgninProfile returns true in the restart case? That seems a bug to me?

https://codereview.chromium.org/16658015/diff/8001/chrome/browser/chromeos/po...
File chrome/browser/chromeos/policy/recommendation_restorer.h (right):

https://codereview.chromium.org/16658015/diff/8001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/recommendation_restorer.h:41: void
CheckPref(const std::string& pref_name);
needs doc

https://codereview.chromium.org/16658015/diff/8001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/recommendation_restorer.h:48:
std::map<std::string, base::Value*> recommended_values_;
what is this good for?

https://codereview.chromium.org/16658015/diff/8001/chrome/browser/policy/conf...
File chrome/browser/policy/configuration_policy_handler.cc (right):

https://codereview.chromium.org/16658015/diff/8001/chrome/browser/policy/conf...
chrome/browser/policy/configuration_policy_handler.cc:296: if
(!strlen(pref_path_))
Why not test for NULL?

https://codereview.chromium.org/16658015/diff/8001/chrome/browser/policy/conf...
File chrome/browser/policy/configuration_policy_handler_list.cc (right):

https://codereview.chromium.org/16658015/diff/8001/chrome/browser/policy/conf...
chrome/browser/policy/configuration_policy_handler_list.cc:29: const char
kNoPref[] = "";
Why not just NULL?

https://codereview.chromium.org/16658015/diff/8001/chrome/browser/policy/prof...
File chrome/browser/policy/profile_policy_connector.cc (right):

https://codereview.chromium.org/16658015/diff/8001/chrome/browser/policy/prof...
chrome/browser/policy/profile_policy_connector.cc:26: #include
"chrome/browser/chromeos/policy/login_profile_policy_provider.cc"
ah, no.

https://codereview.chromium.org/16658015/diff/8001/chrome/browser/policy/prof...
File chrome/browser/policy/profile_policy_connector.h (right):

https://codereview.chromium.org/16658015/diff/8001/chrome/browser/policy/prof...
chrome/browser/policy/profile_policy_connector.h:91:
device_local_account_policy_provider_;
It seems like we can have either login_profile_policy_provider_ or
device_local_account_policy_provider_, but never both of them. We don't seem to
care about the specific type either, so maybe we rather want a single:

scoped_ptr<ConfigurationPolicyProvider> user_policy_provider_

https://codereview.chromium.org/16658015/diff/8001/chrome/test/base/testing_p...
File chrome/test/base/testing_profile_manager.h (right):

https://codereview.chromium.org/16658015/diff/8001/chrome/test/base/testing_p...
chrome/test/base/testing_profile_manager.h:44: // creates a pref store on
demand.
s/pref store/PrefService/

[bartfab (slow), 2013-06-12 18:57:49]

> One thing I don't fully grok is why you
> need these TestingProfile changes?

I need to have the TestingProfileManager construct a
TestingProfile in which particular user prefs are already set so that I can
verify the BCKS clears them during construction.

https://chromiumcodereview.appspot.com/16658015/diff/8001/chrome/app/policy/p...
File chrome/app/policy/policy_templates.json (right):

https://chromiumcodereview.appspot.com/16658015/diff/8001/chrome/app/policy/p...
chrome/app/policy/policy_templates.json:4608: 'name':
'LoginScreenDefaultLargeCursorEnabled',
On 2013/06/12 13:55:16, Mattias Nissler wrote:
> We've named other policy DeviceXYZ, maybe do the same here for consistency?

Done.

https://chromiumcodereview.appspot.com/16658015/diff/8001/chrome/browser/chro...
File chrome/browser/chromeos/policy/device_policy_browsertest.cc (right):

https://chromiumcodereview.appspot.com/16658015/diff/8001/chrome/browser/chro...
chrome/browser/chromeos/policy/device_policy_browsertest.cc:88: class
DevicePolicyBrowsertest : public DevicePolicyCrosBrowserTest,
On 2013/06/12 13:55:16, Mattias Nissler wrote:
> The name of this class and file is a bit misleading. Should probably be
> DevicePolicyDefaultsBrowserTest?

My idea was that we do not have the complete boilerplate for testing device
policies anywhere. I wanted to create a class that would be used for simple
device policy tests in the future, by adding more tests. But as you prefer a
less generic name, I renamed the class.

https://chromiumcodereview.appspot.com/16658015/diff/8001/chrome/browser/chro...
chrome/browser/chromeos/policy/device_policy_browsertest.cc:114: if (logged_in_)
On 2013/06/12 13:55:16, Mattias Nissler wrote:
> It seems like logged_in_ behavior and test verification is vastly different
from
> !logged_in_ behavior. Does it really make sense to treat them the same?

Done.

https://chromiumcodereview.appspot.com/16658015/diff/8001/chrome/browser/chro...
chrome/browser/chromeos/policy/device_policy_browsertest.cc:145:
install_attrs_file));
On 2013/06/12 13:55:16, Mattias Nissler wrote:
> Given this is a copy of existing code, we should probably make it available
via
> DevicePolicyCrosBrowserTest.

Done. But I had to make the method static because the other user,
DeviceLocalAccountTest, does not inherit from DevicePolicyCrosBrowserTest.

https://chromiumcodereview.appspot.com/16658015/diff/8001/chrome/browser/chro...
chrome/browser/chromeos/policy/device_policy_browsertest.cc:197: Profile*
current_profile = logged_in_ ? ProfileManager::GetDefaultProfile() :
On 2013/06/12 13:55:16, Mattias Nissler wrote:
> style nit: not sure, but I think the colon commonly goes on the next line,
> aligned with the ?

Done.

https://chromiumcodereview.appspot.com/16658015/diff/8001/chrome/browser/chro...
chrome/browser/chromeos/policy/device_policy_browsertest.cc:317: }
On 2013/06/12 13:55:16, Mattias Nissler wrote:
> The previous 3 test cases are identical in terms of code (except for the pref
> name and the last line) AFAICS. Can they share code?

Done.

https://chromiumcodereview.appspot.com/16658015/diff/8001/chrome/browser/chro...
File chrome/browser/chromeos/policy/login_profile_policy_provider.cc (right):

https://chromiumcodereview.appspot.com/16658015/diff/8001/chrome/browser/chro...
chrome/browser/chromeos/policy/login_profile_policy_provider.cc:64: return
device_policy_service_->IsInitializationComplete(domain);
On 2013/06/12 13:55:16, Mattias Nissler wrote:
> This might hold startup, have you checked?

Removed so that ConfigurationPolicyProvider::IsInitializationComplete(), which
always returns true, gets called.

https://chromiumcodereview.appspot.com/16658015/diff/8001/chrome/browser/chro...
chrome/browser/chromeos/policy/login_profile_policy_provider.cc:71:
weak_factory_.GetWeakPtr()));
On 2013/06/12 13:55:16, Mattias Nissler wrote:
> The contract is that if you receive a RefreshPolicies() call, the next
> UpdatePolicy() needs to contain the changes pulled in via that. So you should
> probably check weak_factory_.HasWeakPtrs() in UpdateFromDevicePolicy() and
only
> perform the update once the actual callback runs. You'll also want to do a
> weak_factory_.InvalidateWeakPtrs() here.

Done. The correct implementation is slightly different: Checking for
HasWeakPtrs() is not correct because when the callback finally runs, a last weak
pointer still exists. Also, UpdateFromDevicePolicy() needs to be able to tell
whether it was invoked through the callback or via another code path. I modeled
the implementation after your DeviceLocalAccountPolicyProvider, using a bool to
keep track of the status.

https://chromiumcodereview.appspot.com/16658015/diff/8001/chrome/browser/chro...
File chrome/browser/chromeos/policy/recommendation_restorer.cc (right):

https://chromiumcodereview.appspot.com/16658015/diff/8001/chrome/browser/chro...
chrome/browser/chromeos/policy/recommendation_restorer.cc:89: if
(!base::Value::Equals(recommended_value, recommended_value_entry)) {
On 2013/06/12 13:55:16, Mattias Nissler wrote:
> So this recommended_values_ thing is just here to detect whether the pref
change
> was user-triggered, instead of admin-triggered?
> 
> Can't you just ignore pref changes entirely here and just call ClearPref() on
> all prefs when the timer expires? I don't see what would be wrong with that.

No, the |recommended_values_| are here so that I can detect changes to
recommended values. Whenever that happens (when a new policy kicks in or a
policy is modified), I want to restore immediately. When there is a
user-initiated change, I want to start the timer.

https://chromiumcodereview.appspot.com/16658015/diff/8001/chrome/browser/chro...
chrome/browser/chromeos/policy/recommendation_restorer.cc:139:
chromeos::switches::kLoginManager)) {
On 2013/06/12 13:55:16, Mattias Nissler wrote:
> So IsIsgninProfile returns true in the restart case? That seems a bug to me?

IsSigninProfile returns true when the BCKS is created for the login profile. A
login profile is always instantiated in the background. IIRC, it is used for
things beyond the login screen. Hence, it always exists.

https://chromiumcodereview.appspot.com/16658015/diff/8001/chrome/browser/chro...
File chrome/browser/chromeos/policy/recommendation_restorer.h (right):

https://chromiumcodereview.appspot.com/16658015/diff/8001/chrome/browser/chro...
chrome/browser/chromeos/policy/recommendation_restorer.h:41: void
CheckPref(const std::string& pref_name);
On 2013/06/12 13:55:16, Mattias Nissler wrote:
> needs doc

Done.

https://chromiumcodereview.appspot.com/16658015/diff/8001/chrome/browser/chro...
chrome/browser/chromeos/policy/recommendation_restorer.h:48:
std::map<std::string, base::Value*> recommended_values_;
On 2013/06/12 13:55:16, Mattias Nissler wrote:
> what is this good for?

Adeed a comment explaining the purpose.

https://chromiumcodereview.appspot.com/16658015/diff/8001/chrome/browser/poli...
File chrome/browser/policy/configuration_policy_handler.cc (right):

https://chromiumcodereview.appspot.com/16658015/diff/8001/chrome/browser/poli...
chrome/browser/policy/configuration_policy_handler.cc:296: if
(!strlen(pref_path_))
On 2013/06/12 13:55:16, Mattias Nissler wrote:
> Why not test for NULL?

Done.

https://chromiumcodereview.appspot.com/16658015/diff/8001/chrome/browser/poli...
File chrome/browser/policy/configuration_policy_handler_list.cc (right):

https://chromiumcodereview.appspot.com/16658015/diff/8001/chrome/browser/poli...
chrome/browser/policy/configuration_policy_handler_list.cc:29: const char
kNoPref[] = "";
On 2013/06/12 13:55:16, Mattias Nissler wrote:
> Why not just NULL?

Done.

https://chromiumcodereview.appspot.com/16658015/diff/8001/chrome/browser/poli...
File chrome/browser/policy/profile_policy_connector.cc (right):

https://chromiumcodereview.appspot.com/16658015/diff/8001/chrome/browser/poli...
chrome/browser/policy/profile_policy_connector.cc:26: #include
"chrome/browser/chromeos/policy/login_profile_policy_provider.cc"
On 2013/06/12 13:55:16, Mattias Nissler wrote:
> ah, no.

Huh, where did that come from? Deleted.

https://chromiumcodereview.appspot.com/16658015/diff/8001/chrome/browser/poli...
File chrome/browser/policy/profile_policy_connector.h (right):

https://chromiumcodereview.appspot.com/16658015/diff/8001/chrome/browser/poli...
chrome/browser/policy/profile_policy_connector.h:91:
device_local_account_policy_provider_;
On 2013/06/12 13:55:16, Mattias Nissler wrote:
> It seems like we can have either login_profile_policy_provider_ or
> device_local_account_policy_provider_, but never both of them. We don't seem
to
> care about the specific type either, so maybe we rather want a single:
> 
> scoped_ptr<ConfigurationPolicyProvider> user_policy_provider_

Done.

https://chromiumcodereview.appspot.com/16658015/diff/8001/chrome/test/base/te...
File chrome/test/base/testing_profile_manager.h (right):

https://chromiumcodereview.appspot.com/16658015/diff/8001/chrome/test/base/te...
chrome/test/base/testing_profile_manager.h:44: // creates a pref store on
demand.
On 2013/06/12 13:55:16, Mattias Nissler wrote:
> s/pref store/PrefService/

Done.

[Mattias Nissler (ping if slow), 2013-06-13 18:11:45]

Still not convinced about the double bookkeeping on recommended pref values.
Feel free to catch me on IM to discuss.

https://chromiumcodereview.appspot.com/16658015/diff/8001/chrome/browser/chro...
File chrome/browser/chromeos/policy/recommendation_restorer.cc (right):

https://chromiumcodereview.appspot.com/16658015/diff/8001/chrome/browser/chro...
chrome/browser/chromeos/policy/recommendation_restorer.cc:89: if
(!base::Value::Equals(recommended_value, recommended_value_entry)) {
On 2013/06/12 18:57:49, bartfab wrote:
> On 2013/06/12 13:55:16, Mattias Nissler wrote:
> > So this recommended_values_ thing is just here to detect whether the pref
> change
> > was user-triggered, instead of admin-triggered?
> > 
> > Can't you just ignore pref changes entirely here and just call ClearPref()
on
> > all prefs when the timer expires? I don't see what would be wrong with that.
> 
> No, the |recommended_values_| are here so that I can detect changes to
> recommended values. Whenever that happens (when a new policy kicks in or a
> policy is modified), I want to restore immediately. When there is a
> user-initiated change, I want to start the timer.

I think a better implementation for this would be to have PrefService fire a
change notification for the recommended value change (it currently filters it if
the effective value doesn't change).

Then again, why would you want to reset immediately if the policy changes?
UX-wise that's probably worse than just switching to the new default when the
timer fires. If there's no user setting, the switch happens immediately, which
is also fine. So why the complexity to support the behavior you describe?

https://chromiumcodereview.appspot.com/16658015/diff/8001/chrome/browser/chro...
chrome/browser/chromeos/policy/recommendation_restorer.cc:139:
chromeos::switches::kLoginManager)) {
On 2013/06/12 18:57:49, bartfab wrote:
> On 2013/06/12 13:55:16, Mattias Nissler wrote:
> > So IsIsgninProfile returns true in the restart case? That seems a bug to me?
> 
> IsSigninProfile returns true when the BCKS is created for the login profile. A
> login profile is always instantiated in the background. IIRC, it is used for
> things beyond the login screen. Hence, it always exists.

So you're saying the accessibility code picks up pref values from the login
profile even if a session is running? I'm just trying to understand what would
go wrong if this check got removed. The comment doesn't really help...

[bartfab (slow), 2013-06-20 08:40:57]

On 2013/06/13 18:11:45, Mattias Nissler wrote:
> Still not convinced about the double bookkeeping on recommended pref values.
> Feel free to catch me on IM to discuss.

As discussed over IM, I changed the implementation so that the timer is now used
for all pref changes, whether they are user-initiated or changes to the
recommended value.

> 
>
https://chromiumcodereview.appspot.com/16658015/diff/8001/chrome/browser/chro...
> File chrome/browser/chromeos/policy/recommendation_restorer.cc (right):
> 
>
https://chromiumcodereview.appspot.com/16658015/diff/8001/chrome/browser/chro...
> chrome/browser/chromeos/policy/recommendation_restorer.cc:89: if
> (!base::Value::Equals(recommended_value, recommended_value_entry)) {
> On 2013/06/12 18:57:49, bartfab wrote:
> > On 2013/06/12 13:55:16, Mattias Nissler wrote:
> > > So this recommended_values_ thing is just here to detect whether the pref
> > change
> > > was user-triggered, instead of admin-triggered?
> > > 
> > > Can't you just ignore pref changes entirely here and just call ClearPref()
> on
> > > all prefs when the timer expires? I don't see what would be wrong with
that.
> > 
> > No, the |recommended_values_| are here so that I can detect changes to
> > recommended values. Whenever that happens (when a new policy kicks in or a
> > policy is modified), I want to restore immediately. When there is a
> > user-initiated change, I want to start the timer.
> 
> I think a better implementation for this would be to have PrefService fire a
> change notification for the recommended value change (it currently filters it
if
> the effective value doesn't change).
> 
> Then again, why would you want to reset immediately if the policy changes?
> UX-wise that's probably worse than just switching to the new default when the
> timer fires. If there's no user setting, the switch happens immediately, which
> is also fine. So why the complexity to support the behavior you describe?

I removed the code in question, as discussed over IM.

> 
>
https://chromiumcodereview.appspot.com/16658015/diff/8001/chrome/browser/chro...
> chrome/browser/chromeos/policy/recommendation_restorer.cc:139:
> chromeos::switches::kLoginManager)) {
> On 2013/06/12 18:57:49, bartfab wrote:
> > On 2013/06/12 13:55:16, Mattias Nissler wrote:
> > > So IsIsgninProfile returns true in the restart case? That seems a bug to
me?
> > 
> > IsSigninProfile returns true when the BCKS is created for the login profile.
A
> > login profile is always instantiated in the background. IIRC, it is used for
> > things beyond the login screen. Hence, it always exists.
> 
> So you're saying the accessibility code picks up pref values from the login
> profile even if a session is running? I'm just trying to understand what would
> go wrong if this check got removed. The comment doesn't really help...

No, the a11y code does not see the pref values in the login profile until the
next time the login screen is shown. But it is still necessary to react to
policy changes that affect the login profile by clearing the corresponding
prefs.

[bartfab (slow), 2013-06-20 08:43:30]

Hi Mattias,

PTAL. I modified the implementation as discussed over IM. I would still love to
land this for M29.

[Mattias Nissler (ping if slow), 2013-06-20 18:42:10]

LGTM with nits.

https://chromiumcodereview.appspot.com/16658015/diff/58001/chrome/browser/chr...
File chrome/browser/chromeos/policy/login_screen_default_policy_browsertest.cc
(right):

https://chromiumcodereview.appspot.com/16658015/diff/58001/chrome/browser/chr...
chrome/browser/chromeos/policy/login_screen_default_policy_browsertest.cc:41:
em::AccessibilitySettingsProto_ScreenMagnifierType_SCREEN_MAGNIFIER_TYPE_FULL;
nit: indentation

https://chromiumcodereview.appspot.com/16658015/diff/58001/chrome/browser/chr...
chrome/browser/chromeos/policy/login_screen_default_policy_browsertest.cc:202:
::VerifyPrefFollowsRecommendation(
nit: The :: should go on the line before.

https://chromiumcodereview.appspot.com/16658015/diff/58001/chrome/browser/chr...
File chrome/browser/chromeos/policy/recommendation_restorer.cc (right):

https://chromiumcodereview.appspot.com/16658015/diff/58001/chrome/browser/chr...
chrome/browser/chromeos/policy/recommendation_restorer.cc:27: // The amount of
idle time after which recommended values are restored.
nit: no indentation

[bartfab (slow), 2013-06-20 19:35:58]

https://chromiumcodereview.appspot.com/16658015/diff/58001/chrome/browser/chr...
File chrome/browser/chromeos/policy/login_screen_default_policy_browsertest.cc
(right):

https://chromiumcodereview.appspot.com/16658015/diff/58001/chrome/browser/chr...
chrome/browser/chromeos/policy/login_screen_default_policy_browsertest.cc:41:
em::AccessibilitySettingsProto_ScreenMagnifierType_SCREEN_MAGNIFIER_TYPE_FULL;
On 2013/06/20 18:42:10, Mattias Nissler wrote:
> nit: indentation

Done.

https://chromiumcodereview.appspot.com/16658015/diff/58001/chrome/browser/chr...
chrome/browser/chromeos/policy/login_screen_default_policy_browsertest.cc:202:
::VerifyPrefFollowsRecommendation(
On 2013/06/20 18:42:10, Mattias Nissler wrote:
> nit: The :: should go on the line before.

Done.

https://chromiumcodereview.appspot.com/16658015/diff/58001/chrome/browser/chr...
File chrome/browser/chromeos/policy/recommendation_restorer.cc (right):

https://chromiumcodereview.appspot.com/16658015/diff/58001/chrome/browser/chr...
chrome/browser/chromeos/policy/recommendation_restorer.cc:27: // The amount of
idle time after which recommended values are restored.
On 2013/06/20 18:42:10, Mattias Nissler wrote:
> nit: no indentation

Done.

[commit-bot: I haz the power, 2013-06-20 19:37:19]

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

[commit-bot: I haz the power, 2013-06-20 20:59:17]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

[commit-bot: I haz the power, 2013-06-20 23:41:57]

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

[commit-bot: I haz the power, 2013-06-21 01:05:48]

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

[commit-bot: I haz the power, 2013-06-21 08:18:25]

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

[commit-bot: I haz the power, 2013-06-21 08:48:59]

Change committed as 207755

[commit-bot: I haz the power, 2013-06-21 15:28:01]

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

[commit-bot: I haz the power, 2013-06-21 18:41:54]

Change committed as 207889