Show a different banner in chrome://settings for extension-controlled settings.

Show a different banner in chrome://settings for extension-controlled settings.

Remove ManagedPrefsBannerBase and OptionsManagedBannerHandler, as we show the banner purely from JS now.

BUG=88341, 80604
TEST=see bug for manual test

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

[Bernhard Bauer, 2011-07-13 11:22:16]

Please review.

Evan: WebUI
Mattias, Joao: Policy changes
Glen: String changes

Thanks!

http://codereview.chromium.org/7342009/diff/2001/chrome/browser/resources/opt...
File chrome/browser/resources/options/personal_options.js (right):

http://codereview.chromium.org/7342009/diff/2001/chrome/browser/resources/opt...
chrome/browser/resources/options/personal_options.js:100: // TODO(joaodasilva):
remove the radio controls toggles below, once a
Joao, could you take a look whether we should change anything here now?

[Mattias Nissler (ping if slow), 2011-07-13 12:07:58]

LGTM with nits for the policy part.

Also, I wonder whether the extension-controlled warnings should have the same
visual appearance? The yellow bar looks kind of a warning. That doesn't make
sense for extensions, since in this case the user can actually overrule the
extension by just uninstalling it...

http://codereview.chromium.org/7342009/diff/2001/chrome/browser/resources/opt...
File chrome/browser/resources/options/pref_ui.js (right):

http://codereview.chromium.org/7342009/diff/2001/chrome/browser/resources/opt...
chrome/browser/resources/options/pref_ui.js:107: * Whether the preference is
controlled by something else then the user's
s/then/than/ (also below)

http://codereview.chromium.org/7342009/diff/2001/chrome/browser/resources/opt...
chrome/browser/resources/options/pref_ui.js:108: settings (either 'policy' or
'extension').
missing asterisk (also below)

http://codereview.chromium.org/7342009/diff/2001/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/core_options_handler.cc (right):

http://codereview.chromium.org/7342009/diff/2001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/core_options_handler.cc:53:
localized_strings->SetString("policyManagedPrefsBannerText",
Not sure whether it's mandatory, but I personally find code more readable that
aligns the beginning of all parameters to a function. What I'm saying is that
I'd like a line break after the opening parentheses.

[Evan Stade, 2011-07-13 17:06:03]

even though I am not Glen, I gave my opinion on the strings

http://codereview.chromium.org/7342009/diff/2001/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/7342009/diff/2001/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:7829: Some <ph
name="IDS_OPTIONS_PREFS_TERM">$1<ex>options</ex></ph> have been disabled by your
IT administrator.
I don't think this is valid for translations. For example some languages might
have different genders for "options" vs. "preferences".

http://codereview.chromium.org/7342009/diff/2001/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:7832: Some <ph
name="IDS_OPTIONS_PREFS_TERM">$1<ex>options</ex></ph> are being managed by an
extension for you.
Some options are being controlled by an extension.

http://codereview.chromium.org/7342009/diff/2001/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:7835: Some <ph
name="IDS_OPTIONS_PREFS_TERM">$1<ex>options</ex></ph> have been disabled by your
IT administrator, and some are being managed by an extension for you. Lucky you!
Some options have been disabled by your IT administrator and others are being
controlled by an extension.

http://codereview.chromium.org/7342009/diff/2001/chrome/browser/resources/opt...
File chrome/browser/resources/options/options.html (right):

http://codereview.chromium.org/7342009/diff/2001/chrome/browser/resources/opt...
chrome/browser/resources/options/options.html:107: <span
id="policy-managed-prefs-text"
just use one span with the text content controlled in js

http://codereview.chromium.org/7342009/diff/2001/chrome/browser/resources/opt...
File chrome/browser/resources/options/personal_options.js (right):

http://codereview.chromium.org/7342009/diff/2001/chrome/browser/resources/opt...
chrome/browser/resources/options/personal_options.js:280: 
?

http://codereview.chromium.org/7342009/diff/2001/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/core_options_handler.cc (right):

http://codereview.chromium.org/7342009/diff/2001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/core_options_handler.cc:53:
localized_strings->SetString("policyManagedPrefsBannerText",
On 2011/07/13 12:07:58, Mattias Nissler wrote:
> Not sure whether it's mandatory, but I personally find code more readable that
> aligns the beginning of all parameters to a function. What I'm saying is that
> I'd like a line break after the opening parentheses.

no it is not mandatory, both styles are acceptable. I think it's best to match
the precedent set by existing nearby code.

http://codereview.chromium.org/7342009/diff/2001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/core_options_handler.cc:153: if
(pref->IsManaged()) {
no curlies

[Glen Murphy, 2011-07-13 17:23:16]

Agree with Evan with the additional removal of 'IT'.

http://codereview.chromium.org/7342009/diff/2001/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/7342009/diff/2001/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:7829: Some <ph
name="IDS_OPTIONS_PREFS_TERM">$1<ex>options</ex></ph> have been disabled by your
IT administrator.
We can also drop the 'IT' - it's meaningless to many people, and is somewhat
redundant with 'administrator'.

http://codereview.chromium.org/7342009/diff/2001/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:7835: Some <ph
name="IDS_OPTIONS_PREFS_TERM">$1<ex>options</ex></ph> have been disabled by your
IT administrator, and some are being managed by an extension for you. Lucky you!
Evan's without the 'IT'.

[Joao da Silva, 2011-07-13 17:35:17]

http://codereview.chromium.org/7342009/diff/2001/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/7342009/diff/2001/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:7827: 
Nit: trailing whites

http://codereview.chromium.org/7342009/diff/2001/chrome/browser/resources/opt...
File chrome/browser/resources/options/personal_options.js (right):

http://codereview.chromium.org/7342009/diff/2001/chrome/browser/resources/opt...
chrome/browser/resources/options/personal_options.js:100: // TODO(joaodasilva):
remove the radio controls toggles below, once a
On 2011/07/13 11:22:16, Bernhard Bauer wrote:
> Joao, could you take a look whether we should change anything here now?
Yes, see comments below. These temporary fixes should go away once we restore
the enabled/disabled state of the controls when policies are removed.

http://codereview.chromium.org/7342009/diff/2001/chrome/browser/resources/opt...
chrome/browser/resources/options/personal_options.js:110: var managed =
event.value && event.value['managed'];
Replace this line with:
var managed = event.value && event.value['controlledBy'];

http://codereview.chromium.org/7342009/diff/2001/chrome/browser/resources/opt...
chrome/browser/resources/options/personal_options.js:131: var managed =
event.value && event.value['managed'];
Same change on this line too.

http://codereview.chromium.org/7342009/diff/2001/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/core_options_handler.cc (right):

http://codereview.chromium.org/7342009/diff/2001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/core_options_handler.cc:157: }
The code here that creates the DictionaryValue is the same as the code below.
I'd suggest moving it to a common function in the anonymous namespace, and
replacing it here with something similar to
return_value = CreatePrefValue(pref)

WDYT?

http://codereview.chromium.org/7342009/diff/2001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/core_options_handler.cc:398: }
See comment above.

[Bernhard Bauer, 2011-07-13 22:15:12]

http://codereview.chromium.org/7342009/diff/2001/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/7342009/diff/2001/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:7827: 
On 2011/07/13 17:35:17, Joao da Silva wrote:
> Nit: trailing whites

Where?

http://codereview.chromium.org/7342009/diff/2001/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:7829: Some <ph
name="IDS_OPTIONS_PREFS_TERM">$1<ex>options</ex></ph> have been disabled by your
IT administrator.
On 2011/07/13 17:23:16, Glen Murphy wrote:
> We can also drop the 'IT' - it's meaningless to many people, and is somewhat
> redundant with 'administrator'.

Done.

http://codereview.chromium.org/7342009/diff/2001/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:7832: Some <ph
name="IDS_OPTIONS_PREFS_TERM">$1<ex>options</ex></ph> are being managed by an
extension for you.
On 2011/07/13 17:06:04, Evan Stade wrote:
> Some options are being controlled by an extension.

For proxy settings, the text currently reads "Some options are being managed by
an extension", and I prefer managed as well (since the user is using an
extension to manage the settings). Or should we change that as well?

http://codereview.chromium.org/7342009/diff/2001/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:7835: Some <ph
name="IDS_OPTIONS_PREFS_TERM">$1<ex>options</ex></ph> have been disabled by your
IT administrator, and some are being managed by an extension for you. Lucky you!
On 2011/07/13 17:23:16, Glen Murphy wrote:
> Evan's without the 'IT'.

Done.

http://codereview.chromium.org/7342009/diff/2001/chrome/browser/resources/opt...
File chrome/browser/resources/options/options.html (right):

http://codereview.chromium.org/7342009/diff/2001/chrome/browser/resources/opt...
chrome/browser/resources/options/options.html:107: <span
id="policy-managed-prefs-text"
On 2011/07/13 17:06:04, Evan Stade wrote:
> just use one span with the text content controlled in js

Done.

http://codereview.chromium.org/7342009/diff/2001/chrome/browser/resources/opt...
File chrome/browser/resources/options/personal_options.js (right):

http://codereview.chromium.org/7342009/diff/2001/chrome/browser/resources/opt...
chrome/browser/resources/options/personal_options.js:100: // TODO(joaodasilva):
remove the radio controls toggles below, once a
On 2011/07/13 17:35:17, Joao da Silva wrote:
> On 2011/07/13 11:22:16, Bernhard Bauer wrote:
> > Joao, could you take a look whether we should change anything here now?
> Yes, see comments below. These temporary fixes should go away once we restore
> the enabled/disabled state of the controls when policies are removed.

But we do that now (see http://crbug.com/80604).

http://codereview.chromium.org/7342009/diff/2001/chrome/browser/resources/opt...
chrome/browser/resources/options/personal_options.js:280: 
On 2011/07/13 17:06:04, Evan Stade wrote:
> ?

That was a whitespace change to include this file in the CL (so Joao can comment
on it). I'll remove it before committing.

http://codereview.chromium.org/7342009/diff/2001/chrome/browser/resources/opt...
File chrome/browser/resources/options/pref_ui.js (right):

http://codereview.chromium.org/7342009/diff/2001/chrome/browser/resources/opt...
chrome/browser/resources/options/pref_ui.js:107: * Whether the preference is
controlled by something else then the user's
On 2011/07/13 12:07:58, Mattias Nissler wrote:
> s/then/than/ (also below)

Done.

http://codereview.chromium.org/7342009/diff/2001/chrome/browser/resources/opt...
chrome/browser/resources/options/pref_ui.js:108: settings (either 'policy' or
'extension').
On 2011/07/13 12:07:58, Mattias Nissler wrote:
> missing asterisk (also below)

Done.

http://codereview.chromium.org/7342009/diff/2001/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/core_options_handler.cc (right):

http://codereview.chromium.org/7342009/diff/2001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/core_options_handler.cc:153: if
(pref->IsManaged()) {
On 2011/07/13 17:06:04, Evan Stade wrote:
> no curlies

Done.

http://codereview.chromium.org/7342009/diff/2001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/core_options_handler.cc:157: }
On 2011/07/13 17:35:17, Joao da Silva wrote:
> The code here that creates the DictionaryValue is the same as the code below.
> I'd suggest moving it to a common function in the anonymous namespace, and
> replacing it here with something similar to
> return_value = CreatePrefValue(pref)
> 
> WDYT?

Good idea. Done.

http://codereview.chromium.org/7342009/diff/2001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/core_options_handler.cc:398: }
On 2011/07/13 17:35:17, Joao da Silva wrote:
> See comment above.

Done.

[Joao da Silva, 2011-07-14 09:04:11]

LGTM!

http://codereview.chromium.org/7342009/diff/2001/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/7342009/diff/2001/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:7827: 
On 2011/07/13 22:15:12, Bernhard Bauer wrote:
> On 2011/07/13 17:35:17, Joao da Silva wrote:
> > Nit: trailing whites
> 
> Where?
Line 7827. I patched this into my working tree and git diff highlighted it. On
rietveld you can try selecting the whites with the mouse (compare with line
7811).

http://codereview.chromium.org/7342009/diff/2001/chrome/browser/resources/opt...
File chrome/browser/resources/options/personal_options.js (right):

http://codereview.chromium.org/7342009/diff/2001/chrome/browser/resources/opt...
chrome/browser/resources/options/personal_options.js:100: // TODO(joaodasilva):
remove the radio controls toggles below, once a
On 2011/07/13 22:15:12, Bernhard Bauer wrote:
> On 2011/07/13 17:35:17, Joao da Silva wrote:
> > On 2011/07/13 11:22:16, Bernhard Bauer wrote:
> > > Joao, could you take a look whether we should change anything here now?
> > Yes, see comments below. These temporary fixes should go away once we
restore
> > the enabled/disabled state of the controls when policies are removed.
> 
> But we do that now (see http://crbug.com/80604).

Very cool! In that case this code can be fixed, I'll prepare a CL for that. Can
you still include the fixes below in this patch, so that a regression isn't
introduced?

http://codereview.chromium.org/7342009/diff/2001/chrome/browser/resources/opt...
chrome/browser/resources/options/personal_options.js:110: var managed =
event.value && event.value['managed'];
On 2011/07/13 17:35:17, Joao da Silva wrote:
> Replace this line with:
> var managed = event.value && event.value['controlledBy'];

The lines affecting the radio controls ('passwords-offersave' and
'passwords-neversave') can be removed too.

[Bernhard Bauer, 2011-07-14 10:15:56]

http://codereview.chromium.org/7342009/diff/2001/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/7342009/diff/2001/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:7827: 
On 2011/07/14 09:04:11, Joao da Silva wrote:
> On 2011/07/13 22:15:12, Bernhard Bauer wrote:
> > On 2011/07/13 17:35:17, Joao da Silva wrote:
> > > Nit: trailing whites
> > 
> > Where?
> Line 7827. I patched this into my working tree and git diff highlighted it. On
> rietveld you can try selecting the whites with the mouse (compare with line
> 7811).

Ah, got it.

http://codereview.chromium.org/7342009/diff/2001/chrome/browser/resources/opt...
File chrome/browser/resources/options/personal_options.js (right):

http://codereview.chromium.org/7342009/diff/2001/chrome/browser/resources/opt...
chrome/browser/resources/options/personal_options.js:100: // TODO(joaodasilva):
remove the radio controls toggles below, once a
On 2011/07/14 09:04:11, Joao da Silva wrote:
> On 2011/07/13 22:15:12, Bernhard Bauer wrote:
> > On 2011/07/13 17:35:17, Joao da Silva wrote:
> > > On 2011/07/13 11:22:16, Bernhard Bauer wrote:
> > > > Joao, could you take a look whether we should change anything here now?
> > > Yes, see comments below. These temporary fixes should go away once we
> restore
> > > the enabled/disabled state of the controls when policies are removed.
> > 
> > But we do that now (see http://crbug.com/80604).
> 
> Very cool! In that case this code can be fixed, I'll prepare a CL for that.
Can
> you still include the fixes below in this patch, so that a regression isn't
> introduced?

Sure.

http://codereview.chromium.org/7342009/diff/2001/chrome/browser/resources/opt...
chrome/browser/resources/options/personal_options.js:110: var managed =
event.value && event.value['managed'];
On 2011/07/14 09:04:11, Joao da Silva wrote:
> On 2011/07/13 17:35:17, Joao da Silva wrote:
> > Replace this line with:
> > var managed = event.value && event.value['controlledBy'];
> 
> The lines affecting the radio controls ('passwords-offersave' and
> 'passwords-neversave') can be removed too.

Done.

http://codereview.chromium.org/7342009/diff/2001/chrome/browser/resources/opt...
chrome/browser/resources/options/personal_options.js:110: var managed =
event.value && event.value['managed'];
On 2011/07/13 17:35:17, Joao da Silva wrote:
> Replace this line with:
> var managed = event.value && event.value['controlledBy'];

I try to use |event.value['controlledBy']| to store what part of the pref system
controls the preference (for the banner), and |event.value['disabled']| to store
whether the preference is user-modifiable (to disable the UI element itself). In
theory there might be a difference between those two.

http://codereview.chromium.org/7342009/diff/2001/chrome/browser/resources/opt...
chrome/browser/resources/options/personal_options.js:131: var managed =
event.value && event.value['managed'];
On 2011/07/13 17:35:17, Joao da Silva wrote:
> Same change on this line too.

Done.

[Bernhard Bauer, 2011-07-15 08:44:26]

Evan, ping?

On Thu, Jul 14, 2011 at 11:15,  <bauerb@chromium.org> wrote:
>
>
http://codereview.chromium.org/7342009/diff/2001/chrome/app/generated_resourc...
> File chrome/app/generated_resources.grd (right):
>
>
http://codereview.chromium.org/7342009/diff/2001/chrome/app/generated_resourc...
> chrome/app/generated_resources.grd:7827:
> On 2011/07/14 09:04:11, Joao da Silva wrote:
>>
>> On 2011/07/13 22:15:12, Bernhard Bauer wrote:
>> > On 2011/07/13 17:35:17, Joao da Silva wrote:
>> > > Nit: trailing whites
>> >
>> > Where?
>> Line 7827. I patched this into my working tree and git diff
>
> highlighted it. On
>>
>> rietveld you can try selecting the whites with the mouse (compare with
>
> line
>>
>> 7811).
>
> Ah, got it.
>
>
http://codereview.chromium.org/7342009/diff/2001/chrome/browser/resources/opt...
> File chrome/browser/resources/options/personal_options.js (right):
>
>
http://codereview.chromium.org/7342009/diff/2001/chrome/browser/resources/opt...
> chrome/browser/resources/options/personal_options.js:100: //
> TODO(joaodasilva): remove the radio controls toggles below, once a
> On 2011/07/14 09:04:11, Joao da Silva wrote:
>>
>> On 2011/07/13 22:15:12, Bernhard Bauer wrote:
>> > On 2011/07/13 17:35:17, Joao da Silva wrote:
>> > > On 2011/07/13 11:22:16, Bernhard Bauer wrote:
>> > > > Joao, could you take a look whether we should change anything
>
> here now?
>>
>> > > Yes, see comments below. These temporary fixes should go away once
>
> we
>>
>> restore
>> > > the enabled/disabled state of the controls when policies are
>
> removed.
>>
>> >
>> > But we do that now (see http://crbug.com/80604).
>
>> Very cool! In that case this code can be fixed, I'll prepare a CL for
>
> that. Can
>>
>> you still include the fixes below in this patch, so that a regression
>
> isn't
>>
>> introduced?
>
> Sure.
>
>
http://codereview.chromium.org/7342009/diff/2001/chrome/browser/resources/opt...
> chrome/browser/resources/options/personal_options.js:110: var managed =
> event.value && event.value['managed'];
> On 2011/07/14 09:04:11, Joao da Silva wrote:
>>
>> On 2011/07/13 17:35:17, Joao da Silva wrote:
>> > Replace this line with:
>> > var managed = event.value && event.value['controlledBy'];
>
>> The lines affecting the radio controls ('passwords-offersave' and
>> 'passwords-neversave') can be removed too.
>
> Done.
>
>
http://codereview.chromium.org/7342009/diff/2001/chrome/browser/resources/opt...
> chrome/browser/resources/options/personal_options.js:110: var managed =
> event.value && event.value['managed'];
> On 2011/07/13 17:35:17, Joao da Silva wrote:
>>
>> Replace this line with:
>> var managed = event.value && event.value['controlledBy'];
>
> I try to use |event.value['controlledBy']| to store what part of the
> pref system controls the preference (for the banner), and
> |event.value['disabled']| to store whether the preference is
> user-modifiable (to disable the UI element itself). In theory there
> might be a difference between those two.
>
>
http://codereview.chromium.org/7342009/diff/2001/chrome/browser/resources/opt...
> chrome/browser/resources/options/personal_options.js:131: var managed =
> event.value && event.value['managed'];
> On 2011/07/13 17:35:17, Joao da Silva wrote:
>>
>> Same change on this line too.
>
> Done.
>
> http://codereview.chromium.org/7342009/
>

[Evan Stade, 2011-07-15 19:19:10]

sorry for slowness.

I don't have an opinion on "managed" vs. "controlled" as a UI string, but
whatever we choose should be consistent throughout the UI.

LGTM

http://codereview.chromium.org/7342009/diff/13001/chrome/browser/resources/op...
File chrome/browser/resources/options/options_page.js (right):

http://codereview.chromium.org/7342009/diff/13001/chrome/browser/resources/op...
chrome/browser/resources/options/options_page.js:836:
$('managed-prefs-text').innerText =
the standard is actually textContent (I'm sure we use innerText elsewhere but
might as well stick to the standard)