Disable chrome://options UI elements if the corresponding preference is not user-modifiable.

Disable chrome://options UI elements if the corresponding preference is not user-modifiable.

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

[Bernhard Bauer, 2011-07-06 12:14:19]

please review.

[Evan Stade, 2011-07-06 19:44:57]

http://codereview.chromium.org/7310003/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/pref_ui.js (right):

http://codereview.chromium.org/7310003/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/pref_ui.js:27: while (node) {
use findAncestor from util.js

http://codereview.chromium.org/7310003/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/pref_ui.js:38: el.disabled = true;
does this work?

el.disabled = event.value['disabled']

[Bernhard Bauer, 2011-07-06 19:55:57]

http://codereview.chromium.org/7310003/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/pref_ui.js (right):

http://codereview.chromium.org/7310003/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/pref_ui.js:27: while (node) {
On 2011/07/06 19:44:57, Evan Stade wrote:
> use findAncestor from util.js

Ahh, nice. Done.

http://codereview.chromium.org/7310003/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/pref_ui.js:38: el.disabled = true;
On 2011/07/06 19:44:57, Evan Stade wrote:
> does this work?
> 
> el.disabled = event.value['disabled']

I think elements could already be disabled for other reasons, and we wouldn't
want to enable them in that case.

[Evan Stade, 2011-07-06 20:12:43]

http://codereview.chromium.org/7310003/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/pref_ui.js (right):

http://codereview.chromium.org/7310003/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/pref_ui.js:38: el.disabled = true;
On 2011/07/06 19:55:57, Bernhard Bauer wrote:
> On 2011/07/06 19:44:57, Evan Stade wrote:
> > does this work?
> > 
> > el.disabled = event.value['disabled']
> 
> I think elements could already be disabled for other reasons, and we wouldn't
> want to enable them in that case. 

true, but what if a user uninstalls an extension -- shouldn't this go back to
false? (and shouldn't the label title go back to empty)

[Bernhard Bauer, 2011-07-07 08:45:56]

http://codereview.chromium.org/7310003/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/pref_ui.js (right):

http://codereview.chromium.org/7310003/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/pref_ui.js:38: el.disabled = true;
On 2011/07/06 20:12:43, Evan Stade wrote:
> On 2011/07/06 19:55:57, Bernhard Bauer wrote:
> > On 2011/07/06 19:44:57, Evan Stade wrote:
> > > does this work?
> > > 
> > > el.disabled = event.value['disabled']
> > 
> > I think elements could already be disabled for other reasons, and we
wouldn't
> > want to enable them in that case. 
> 
> true, but what if a user uninstalls an extension -- shouldn't this go back to
> false? (and shouldn't the label title go back to empty)

Done.

[Evan Stade, 2011-07-07 20:06:45]

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

http://codereview.chromium.org/7310003/diff/6001/chrome/browser/resources/opt...
chrome/browser/resources/options/pref_ui.js:31: null;
this will overwrite whatever other title the label may have had, but I guess we
don't use title elsewhere

http://codereview.chromium.org/7310003/diff/6001/chrome/browser/resources/opt...
chrome/browser/resources/options/pref_ui.js:42: el.disabled =
event.value['disabled'];
this still doesn't seem very foolproof. reenable could be stale (the preference
*was* disabled somewhere else, but no longer is). Actually if that happens then
probably |disabled| has been overwritten elsewhere. I don't see an easy way out
of this... wdyt?

[Bernhard Bauer, 2011-07-07 20:45:11]

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

http://codereview.chromium.org/7310003/diff/6001/chrome/browser/resources/opt...
chrome/browser/resources/options/pref_ui.js:31: null;
On 2011/07/07 20:06:45, Evan Stade wrote:
> this will overwrite whatever other title the label may have had, but I guess
we
> don't use title elsewhere

Per the UI leads, we're going to add an infobar like the one for policy-managed
settings in this case as well, so I guess the title won't be necessary any more.
OTOH, that's going to be a somewhat bigger change, so I'd like to get the
disabling in first.

http://codereview.chromium.org/7310003/diff/6001/chrome/browser/resources/opt...
chrome/browser/resources/options/pref_ui.js:42: el.disabled =
event.value['disabled'];
On 2011/07/07 20:06:45, Evan Stade wrote:
> this still doesn't seem very foolproof. 

I know :-/ This seemed like the easiest way without having to change every other
place that disables UI elements. 

> reenable could be stale (the preference
> *was* disabled somewhere else, but no longer is). Actually if that happens
then
> probably |disabled| has been overwritten elsewhere. 

That would be an error at the other place in the code if it reenables a UI
element without checking if it was disabled for other reasons, but I don't think
that happens (most of the time, elements are just disabled, and in that case
|reenable| defaults to the right value).

> I don't see an easy way out
> of this... wdyt?

I think this is at least correct in more cases than the current behaviour, where
we don't reenable elements at all if they become unmanaged. If there are cases
where we wrongly reenable elements from other places, we should identify and fix
them separately.

[Evan Stade, 2011-07-07 21:09:12]

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

http://codereview.chromium.org/7310003/diff/6001/chrome/browser/resources/opt...
chrome/browser/resources/options/pref_ui.js:42: el.disabled =
event.value['disabled'];
On 2011/07/07 20:45:14, Bernhard Bauer wrote:
> On 2011/07/07 20:06:45, Evan Stade wrote:
> > this still doesn't seem very foolproof. 
> 
> I know :-/ This seemed like the easiest way without having to change every
other
> place that disables UI elements. 
> 
> > reenable could be stale (the preference
> > *was* disabled somewhere else, but no longer is). Actually if that happens
> then
> > probably |disabled| has been overwritten elsewhere. 
> 
> That would be an error at the other place in the code if it reenables a UI
> element without checking if it was disabled for other reasons,

but how could the code check if it were disabled for other reasons? That fact
isn't stored anywhere, except in 'reenable', and from outside the context of
this function that would be a pretty opaque name. Maybe change the name to
notUserControlled or something

> but I don't think
> that happens (most of the time, elements are just disabled, and in that case
> |reenable| defaults to the right value).
> 
> > I don't see an easy way out
> > of this... wdyt?
> 
> I think this is at least correct in more cases than the current behaviour,
where
> we don't reenable elements at all if they become unmanaged. If there are cases
> where we wrongly reenable elements from other places, we should identify and
fix
> them separately.

I don't know how likely it is to run into this in the wild, so while I agree, I
do not think we should wait for bug reports to roll in.

[Bernhard Bauer, 2011-07-07 21:33:35]

On 2011/07/07 21:09:12, Evan Stade wrote:
>
http://codereview.chromium.org/7310003/diff/6001/chrome/browser/resources/opt...
> File chrome/browser/resources/options/pref_ui.js (right):
> 
>
http://codereview.chromium.org/7310003/diff/6001/chrome/browser/resources/opt...
> chrome/browser/resources/options/pref_ui.js:42: el.disabled =
> event.value['disabled'];
> On 2011/07/07 20:45:14, Bernhard Bauer wrote:
> > On 2011/07/07 20:06:45, Evan Stade wrote:
> > > this still doesn't seem very foolproof. 
> > 
> > I know :-/ This seemed like the easiest way without having to change every
> other
> > place that disables UI elements. 
> > 
> > > reenable could be stale (the preference
> > > *was* disabled somewhere else, but no longer is). Actually if that happens
> > then
> > > probably |disabled| has been overwritten elsewhere. 
> > 
> > That would be an error at the other place in the code if it reenables a UI
> > element without checking if it was disabled for other reasons,
> 
> but how could the code check if it were disabled for other reasons? That fact
> isn't stored anywhere, except in 'reenable', and from outside the context of
> this function that would be a pretty opaque name. Maybe change the name to
> notUserControlled or something

Sure, I can do that.

> > but I don't think
> > that happens (most of the time, elements are just disabled, and in that case
> > |reenable| defaults to the right value).
> > 
> > > I don't see an easy way out
> > > of this... wdyt?
> > 
> > I think this is at least correct in more cases than the current behaviour,
> where
> > we don't reenable elements at all if they become unmanaged. If there are
cases
> > where we wrongly reenable elements from other places, we should identify and
> fix
> > them separately.
> 
> I don't know how likely it is to run into this in the wild, so while I agree,
I
> do not think we should wait for bug reports to roll in.

To put it differently: If bugs of this sort exist, they already exist in the
current version, so I'm not making things any worse here, I'm just not
preventing other people from breaking stuff.

[Evan Stade, 2011-07-07 21:48:18]

lgtm

-- Evan Stade



On Thu, Jul 7, 2011 at 2:33 PM,  <bauerb@chromium.org> wrote:
> On 2011/07/07 21:09:12, Evan Stade wrote:
>
>
http://codereview.chromium.org/7310003/diff/6001/chrome/browser/resources/opt...
>>
>> File chrome/browser/resources/options/pref_ui.js (right):
>
>
>
http://codereview.chromium.org/7310003/diff/6001/chrome/browser/resources/opt...
>>
>> chrome/browser/resources/options/pref_ui.js:42: el.disabled =
>> event.value['disabled'];
>> On 2011/07/07 20:45:14, Bernhard Bauer wrote:
>> > On 2011/07/07 20:06:45, Evan Stade wrote:
>> > > this still doesn't seem very foolproof.
>> >
>> > I know :-/ This seemed like the easiest way without having to change
>> > every
>> other
>> > place that disables UI elements.
>> >
>> > > reenable could be stale (the preference
>> > > *was* disabled somewhere else, but no longer is). Actually if that
>> > > happens
>> > then
>> > > probably |disabled| has been overwritten elsewhere.
>> >
>> > That would be an error at the other place in the code if it reenables a
>> > UI
>> > element without checking if it was disabled for other reasons,
>
>> but how could the code check if it were disabled for other reasons? That
>> fact
>> isn't stored anywhere, except in 'reenable', and from outside the context
>> of
>> this function that would be a pretty opaque name. Maybe change the name to
>> notUserControlled or something
>
> Sure, I can do that.
>
>> > but I don't think
>> > that happens (most of the time, elements are just disabled, and in that
>> > case
>> > |reenable| defaults to the right value).
>> >
>> > > I don't see an easy way out
>> > > of this... wdyt?
>> >
>> > I think this is at least correct in more cases than the current
>> > behaviour,
>> where
>> > we don't reenable elements at all if they become unmanaged. If there are
>
> cases
>>
>> > where we wrongly reenable elements from other places, we should identify
>> > and
>> fix
>> > them separately.
>
>> I don't know how likely it is to run into this in the wild, so while I
>> agree,
>
> I
>>
>> do not think we should wait for bug reports to roll in.
>
> To put it differently: If bugs of this sort exist, they already exist in the
> current version, so I'm not making things any worse here, I'm just not
> preventing other people from breaking stuff.
>
> http://codereview.chromium.org/7310003/
>