Add first batch of controlled setting indicators

Add first batch of controlled setting indicators

This CL adds controlled settings indicators to the settings UI homepage
overlay that indicate which settings exactly are enforced or recommended
through policy. Follow-up CLs will add indicators to all other settings
pages as well.

The CL also adds a browser test verifying that the indicators show
enforced/recommended state as expected.

BUG=104955
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=160880

[bartfab (slow), 2012-10-09 11:35:29]

Hi, could you please have a look?

James, this adds the first controlled setting indicators to the options UI.
Joao, this extends your browser tests to also exercise the indicators.

[Joao da Silva, 2012-10-09 14:09:14]

Please submit a bot run. The policy tests look mostly good.

http://codereview.chromium.org/11096020/diff/1/chrome/browser/policy/policy_p...
File chrome/browser/policy/policy_prefs_browsertest.cc (right):

http://codereview.chromium.org/11096020/diff/1/chrome/browser/policy/policy_p...
chrome/browser/policy/policy_prefs_browsertest.cc:94: bool can_be_recommended()
const { return can_be_recommended_; }
Set a default value in the ctor

http://codereview.chromium.org/11096020/diff/1/chrome/browser/policy/policy_p...
chrome/browser/policy/policy_prefs_browsertest.cc:103: const
ScopedVector<IndicatorTestCase>& indicator_test_cases() const {
nit: newline before this line

http://codereview.chromium.org/11096020/diff/1/chrome/browser/policy/policy_p...
chrome/browser/policy/policy_prefs_browsertest.cc:233:
!indicator_test_dict->GetDictionary("policy", &policy))
nit: braces

http://codereview.chromium.org/11096020/diff/1/chrome/browser/policy/policy_p...
chrome/browser/policy/policy_prefs_browsertest.cc:240: readonly));
nitty nit/matter of preference: I find it more readable to move "new
IndicatorTestCase(...));" to a newline with 4-spaces offset. Your choice :-)

http://codereview.chromium.org/11096020/diff/1/chrome/browser/policy/policy_p...
chrome/browser/policy/policy_prefs_browsertest.cc:259: flag = false;
nit: this line can be removed

http://codereview.chromium.org/11096020/diff/1/chrome/browser/policy/policy_p...
chrome/browser/policy/policy_prefs_browsertest.cc:315:
ASSERT_TRUE(content::ExecuteJavaScriptAndExtractString(
ASSERT_* will add a failure and just return from this function. The call sites
must be wrapped with
ASSERT_NO_FATAL_FAILURE(VerifyControlledSettingIndicators(...)) to make the test
body return immediately as well. For the details see

http://code.google.com/p/googletest/wiki/AdvancedGuide#Asserting_on_Subroutines

http://codereview.chromium.org/11096020/diff/1/chrome/browser/policy/policy_p...
chrome/browser/policy/policy_prefs_browsertest.cc:318: const base::ListValue*
indicators;
initialize to NULL

http://codereview.chromium.org/11096020/diff/1/chrome/browser/policy/policy_p...
chrome/browser/policy/policy_prefs_browsertest.cc:326: const
base::DictionaryValue* properties;
nit: initialize to NULL

http://codereview.chromium.org/11096020/diff/1/chrome/browser/policy/policy_p...
chrome/browser/policy/policy_prefs_browsertest.cc:496: indicator_test_case =
indicator_test_cases.begin();
nit: indent (+4 spaces)

[bartfab (slow), 2012-10-09 14:33:02]

http://codereview.chromium.org/11096020/diff/1/chrome/browser/policy/policy_p...
File chrome/browser/policy/policy_prefs_browsertest.cc (right):

http://codereview.chromium.org/11096020/diff/1/chrome/browser/policy/policy_p...
chrome/browser/policy/policy_prefs_browsertest.cc:94: bool can_be_recommended()
const { return can_be_recommended_; }
On 2012/10/09 14:09:14, Joao da Silva wrote:
> Set a default value in the ctor

Done.

http://codereview.chromium.org/11096020/diff/1/chrome/browser/policy/policy_p...
chrome/browser/policy/policy_prefs_browsertest.cc:103: const
ScopedVector<IndicatorTestCase>& indicator_test_cases() const {
On 2012/10/09 14:09:14, Joao da Silva wrote:
> nit: newline before this line

Done.

http://codereview.chromium.org/11096020/diff/1/chrome/browser/policy/policy_p...
chrome/browser/policy/policy_prefs_browsertest.cc:233:
!indicator_test_dict->GetDictionary("policy", &policy))
On 2012/10/09 14:09:14, Joao da Silva wrote:
> nit: braces

Done.

http://codereview.chromium.org/11096020/diff/1/chrome/browser/policy/policy_p...
chrome/browser/policy/policy_prefs_browsertest.cc:240: readonly));
On 2012/10/09 14:09:14, Joao da Silva wrote:
> nitty nit/matter of preference: I find it more readable to move "new
> IndicatorTestCase(...));" to a newline with 4-spaces offset. Your choice :-)

Done.

http://codereview.chromium.org/11096020/diff/1/chrome/browser/policy/policy_p...
chrome/browser/policy/policy_prefs_browsertest.cc:259: flag = false;
On 2012/10/09 14:09:14, Joao da Silva wrote:
> nit: this line can be removed

Done.

http://codereview.chromium.org/11096020/diff/1/chrome/browser/policy/policy_p...
chrome/browser/policy/policy_prefs_browsertest.cc:315:
ASSERT_TRUE(content::ExecuteJavaScriptAndExtractString(
I know. This is what I want: An ASSERT_* failure should stop this execution of
the function but there is no reason to abort the test body. The body is testing
several scenarios (no policy, enforced, recommended, recommended+overridden). If
one of them fails, the other scenarios are not affected and can still be tested.

http://codereview.chromium.org/11096020/diff/1/chrome/browser/policy/policy_p...
chrome/browser/policy/policy_prefs_browsertest.cc:318: const base::ListValue*
indicators;
On 2012/10/09 14:09:14, Joao da Silva wrote:
> initialize to NULL

Done.

http://codereview.chromium.org/11096020/diff/1/chrome/browser/policy/policy_p...
chrome/browser/policy/policy_prefs_browsertest.cc:326: const
base::DictionaryValue* properties;
On 2012/10/09 14:09:14, Joao da Silva wrote:
> nit: initialize to NULL

Done.

http://codereview.chromium.org/11096020/diff/1/chrome/browser/policy/policy_p...
chrome/browser/policy/policy_prefs_browsertest.cc:496: indicator_test_case =
indicator_test_cases.begin();
I originally had 4 spaces but changed them to 5 (aligning inside the bracket) to
be consistent with the remainder of the file, see lines 189 and 386.

[Joao da Silva, 2012-10-09 15:00:12]

policy stuff lgtm

http://codereview.chromium.org/11096020/diff/1/chrome/browser/policy/policy_p...
File chrome/browser/policy/policy_prefs_browsertest.cc (right):

http://codereview.chromium.org/11096020/diff/1/chrome/browser/policy/policy_p...
chrome/browser/policy/policy_prefs_browsertest.cc:315:
ASSERT_TRUE(content::ExecuteJavaScriptAndExtractString(
On 2012/10/09 14:33:02, bartfab wrote:
> I know. This is what I want: An ASSERT_* failure should stop this execution of
> the function but there is no reason to abort the test body. The body is
testing
> several scenarios (no policy, enforced, recommended, recommended+overridden).
If
> one of them fails, the other scenarios are not affected and can still be
tested.

I see, makes sense.

http://codereview.chromium.org/11096020/diff/1/chrome/browser/policy/policy_p...
chrome/browser/policy/policy_prefs_browsertest.cc:496: indicator_test_case =
indicator_test_cases.begin();
On 2012/10/09 14:33:02, bartfab wrote:
> I originally had 4 spaces but changed them to 5 (aligning inside the bracket)
to
> be consistent with the remainder of the file, see lines 189 and 386.

I meant 5 + 4, because the 2nd line is a continuation of the 1st but since it
currently is aligned with it, it looks like a for() with 4 statements inside.
It's not visually clear which line is a continuation of which.

[bartfab (slow), 2012-10-09 15:02:24]

http://codereview.chromium.org/11096020/diff/1/chrome/browser/policy/policy_p...
File chrome/browser/policy/policy_prefs_browsertest.cc (right):

http://codereview.chromium.org/11096020/diff/1/chrome/browser/policy/policy_p...
chrome/browser/policy/policy_prefs_browsertest.cc:496: indicator_test_case =
indicator_test_cases.begin();
On 2012/10/09 15:00:13, Joao da Silva wrote:
> On 2012/10/09 14:33:02, bartfab wrote:
> > I originally had 4 spaces but changed them to 5 (aligning inside the
bracket)
> to
> > be consistent with the remainder of the file, see lines 189 and 386.
> 
> I meant 5 + 4, because the 2nd line is a continuation of the 1st but since it
> currently is aligned with it, it looks like a for() with 4 statements inside.
> It's not visually clear which line is a continuation of which.

Done.

[James Hawkins, 2012-10-09 15:17:11]

LGTM with nit.

http://codereview.chromium.org/11096020/diff/7002/chrome/browser/resources/op...
File chrome/browser/resources/options/home_page_overlay.html (right):

http://codereview.chromium.org/11096020/diff/7002/chrome/browser/resources/op...
chrome/browser/resources/options/home_page_overlay.html:23: value="false"
dialog-pref></span>
nit: Closing brace for a wrapped element must be on a new line.

Here and elsewhere.

[bartfab (slow), 2012-10-09 15:22:06]

http://codereview.chromium.org/11096020/diff/7002/chrome/browser/resources/op...
File chrome/browser/resources/options/home_page_overlay.html (right):

http://codereview.chromium.org/11096020/diff/7002/chrome/browser/resources/op...
chrome/browser/resources/options/home_page_overlay.html:23: value="false"
dialog-pref></span>
On 2012/10/09 15:17:11, James Hawkins wrote:
> nit: Closing brace for a wrapped element must be on a new line.
> 
> Here and elsewhere.

Done.

[commit-bot: I haz the power, 2012-10-09 16:17:26]

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

[commit-bot: I haz the power, 2012-10-09 18:23:39]

Change committed as 160880