Finish implementation of controlled setting indicator

Finish implementation of controlled setting indicator

This CL completes the implementation of a controlled setting indicator
class for Chrome's settings UI. This class can be used to show an icon
next to each setting whose value is enforced or recommended by policy or
an extension. Clicking on it or selecting the indicator by keyboard brings
up an informative bubble.

The CL ensures that indicators already used in the Chrome OS network
settings remain functional. No further indicators are added to the
settings UI - this will be done in follow-up CLs.

BUG=104955
TEST=Chrome OS VM with ONC-configured VPN; only here is a controlled
TEST=setting indicator currently being used (see crbug.com/137470)


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

[bartfab (slow), 2012-10-04 15:27:50]

Hi everyone, could you please have a look?

* James, this completes the implementation of controlled settings indicators in
the UI code.
* Nikita, Chrome OS specific settings UI is touched.
* Philipp, this touches the network setting indicators that you most recently
worked on.

[Nikita (slow), 2012-10-04 16:10:37]

chromeos options lgtm

[James Hawkins, 2012-10-04 16:16:51]

https://codereview.chromium.org/11066015/diff/1/chrome/browser/resources/opti...
File chrome/browser/resources/options/chromeos/internet_detail.js (right):

https://codereview.chromium.org/11066015/diff/1/chrome/browser/resources/opti...
chrome/browser/resources/options/chromeos/internet_detail.js:996: if (propName
&& data[propName]) {
Optional nit: Save a lot of indentation by continuing early on the reverse
condition.

https://codereview.chromium.org/11066015/diff/1/chrome/browser/resources/opti...
File chrome/browser/resources/options/controlled_setting.js (right):

https://codereview.chromium.org/11066015/diff/1/chrome/browser/resources/opti...
chrome/browser/resources/options/controlled_setting.js:146: // Work out the
bubble text.
Can we change the wording from "Work out" to, say, "Construct"?

https://codereview.chromium.org/11066015/diff/1/chrome/browser/resources/opti...
chrome/browser/resources/options/controlled_setting.js:209: * The kind of input
element that this indicator refers to.
I think we need to make this more coupled to the element by having a
for="element-id" attribute.  Query that value for the input element and get its
type.  This will help prevent bugs where the input element type changes but the
type= on the ControlledSettingIndicator isn't changed.  What do you think?

[bartfab (slow), 2012-10-04 18:55:39]

https://chromiumcodereview.appspot.com/11066015/diff/1/chrome/browser/resourc...
File chrome/browser/resources/options/chromeos/internet_detail.js (right):

https://chromiumcodereview.appspot.com/11066015/diff/1/chrome/browser/resourc...
chrome/browser/resources/options/chromeos/internet_detail.js:996: if (propName
&& data[propName]) {
On 2012/10/04 16:16:51, James Hawkins wrote:
> Optional nit: Save a lot of indentation by continuing early on the reverse
> condition.

Done.

https://chromiumcodereview.appspot.com/11066015/diff/1/chrome/browser/resourc...
File chrome/browser/resources/options/controlled_setting.js (right):

https://chromiumcodereview.appspot.com/11066015/diff/1/chrome/browser/resourc...
chrome/browser/resources/options/controlled_setting.js:146: // Work out the
bubble text.
On 2012/10/04 16:16:51, James Hawkins wrote:
> Can we change the wording from "Work out" to, say, "Construct"?

Done.

https://chromiumcodereview.appspot.com/11066015/diff/1/chrome/browser/resourc...
chrome/browser/resources/options/controlled_setting.js:209: * The kind of input
element that this indicator refers to.
I discussed this with Philipp at length. We think that the opposite route is
more desirable, completely decoupling indicators and input elements. Most
indicators will correspond 1:1 to an input element. Some will have more complex
relationships.

It is much cleaner and maintainable if inputs and indicators do not know
anything about each other and do not affect each other directly. Instead, they
can both communicate with the central Preferences class that accepts change
requests and dispatches change events.

I modified the settings indicator now so that it still has a "value" property
that allows it to be visible for certain values of a pref only but did away with
the "type" property completely.

[pneubeck (no reviews), 2012-10-04 18:56:10]

Looks good.

https://codereview.chromium.org/11066015/diff/1/chrome/browser/resources/opti...
File chrome/browser/resources/options/controlled_setting.js (right):

https://codereview.chromium.org/11066015/diff/1/chrome/browser/resources/opti...
chrome/browser/resources/options/controlled_setting.js:46: set
resetHandler(handler) {
Please make it clear in the comment with which 'this' object the handler is
called.

https://codereview.chromium.org/11066015/diff/1/chrome/browser/resources/opti...
chrome/browser/resources/options/controlled_setting.js:155: // No controller, no
popup.
Nit: bubble

https://codereview.chromium.org/11066015/diff/1/chrome/browser/resources/opti...
chrome/browser/resources/options/controlled_setting.js:165: // Create the DOM
tree.
In general, instead of generating a DOM tree, I would prefer adding explicit
"template" html elements that are moved/copied when needed. However as discussed
with Bartosz, there does not seem to be a solution to this with more advantages
than disadvantages.

https://codereview.chromium.org/11066015/diff/1/chrome/browser/resources/opti...
chrome/browser/resources/options/controlled_setting.js:209: * The kind of input
element that this indicator refers to.
For comparison, I like the alternative in the opposite direction. Drop the
'type' field and allow the 'value' field in general with the meaning: this
indicator is only visible/active if the provided value equals the mandatory or
recommended one.

This makes the indicator well-defined and independent of any input-control
implementation.

[bartfab (slow), 2012-10-04 19:17:15]

https://chromiumcodereview.appspot.com/11066015/diff/1/chrome/browser/resourc...
File chrome/browser/resources/options/controlled_setting.js (right):

https://chromiumcodereview.appspot.com/11066015/diff/1/chrome/browser/resourc...
chrome/browser/resources/options/controlled_setting.js:46: set
resetHandler(handler) {
On 2012/10/04 18:56:11, pneubeck wrote:
> Please make it clear in the comment with which 'this' object the handler is
> called.

Done.

https://chromiumcodereview.appspot.com/11066015/diff/1/chrome/browser/resourc...
chrome/browser/resources/options/controlled_setting.js:155: // No controller, no
popup.
On 2012/10/04 18:56:11, pneubeck wrote:
> Nit: bubble

Done.

https://chromiumcodereview.appspot.com/11066015/diff/1/chrome/browser/resourc...
chrome/browser/resources/options/controlled_setting.js:165: // Create the DOM
tree.
Yes, this would be cleaner. I discussed this kind of issue in general with Evan
a few weeks back. If my understanding of the current best practice is correct,
it is acceptable to construct bits of DOM on the fly as long as they are tiny
and trivial (one text div, one link in this case).

[James Hawkins, 2012-10-04 21:57:59]

LGTM, thanks.

[commit-bot: I haz the power, 2012-10-05 07:44:26]

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

[commit-bot: I haz the power, 2012-10-05 12:47:30]

Change committed as 160351

[Vitaly Pavlenko, 2014-09-15 22:35:51]

vitalyp@chromium.org changed reviewers:
+ vitalyp@chromium.org

[Vitaly Pavlenko, 2014-09-15 22:35:52]

https://chromiumcodereview.appspot.com/11066015/diff/1008/chrome/browser/reso...
File chrome/browser/resources/options/controlled_setting.js (right):

https://chromiumcodereview.appspot.com/11066015/diff/1008/chrome/browser/reso...
chrome/browser/resources/options/controlled_setting.js:89: this.controlledBy =
null;
Hi,

I'm compiling Chrome JS with Closure Compiler. Property |controlledBy| is of
type (string|null) here, but we trying to remove null values from properties
defined by cr.defineProperty(). So I tried to rewrite this line with

this.controlledBy = '';

However, that didn't work and caused a test failure:
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...

I don't see any connection between this variable and test failure. 

So if you remember this code, please help me with to answer either question:

a) Is it possible to use empty string for |controlledBy| instead of null? What
should I rewrite besides this file?

b) Could you suggest, how is that test failure connected with my change?

Thank you,
Vitaly

[bartfab (slow), 2014-09-16 14:26:08]

https://chromiumcodereview.appspot.com/11066015/diff/1008/chrome/browser/reso...
File chrome/browser/resources/options/controlled_setting.js (right):

https://chromiumcodereview.appspot.com/11066015/diff/1008/chrome/browser/reso...
chrome/browser/resources/options/controlled_setting.js:89: this.controlledBy =
null;
On 2014/09/15 22:35:51, Vitaly Pavlenko wrote:
> Hi,
> 
> I'm compiling Chrome JS with Closure Compiler. Property |controlledBy| is of
> type (string|null) here, but we trying to remove null values from properties
> defined by cr.defineProperty(). So I tried to rewrite this line with
> 
> this.controlledBy = '';
> 
> However, that didn't work and caused a test failure:
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...
> 
> I don't see any connection between this variable and test failure. 
> 
> So if you remember this code, please help me with to answer either question:
> 
> a) Is it possible to use empty string for |controlledBy| instead of null? What
> should I rewrite besides this file?
> 
> b) Could you suggest, how is that test failure connected with my change?
> 
> Thank you,
> Vitaly

|controlledBy| controls two things:

1) If the property is non-null, a certain UI element (the indicator) will be
shown.
2) The actual property value determines how the indicator behaves.

Your code is triggering 1), which makes the test fail. You can change the code
to treat '' like it treats null now.

However, note that the |controlledBy| value is copied from an event and it may
be null there. If you want to find all the places that fire this event, look for
the comment "Create a synthetic pref change event decorated as
CoreOptionsHandler::CreateValueForPref() does." I marked all the places in the
code that fire such events in this way.

[Vitaly Pavlenko, 2014-09-16 18:11:40]

On 2014/09/16 14:26:08, bartfab wrote:
>
https://chromiumcodereview.appspot.com/11066015/diff/1008/chrome/browser/reso...
> File chrome/browser/resources/options/controlled_setting.js (right):
> 
>
https://chromiumcodereview.appspot.com/11066015/diff/1008/chrome/browser/reso...
> chrome/browser/resources/options/controlled_setting.js:89: this.controlledBy =
> null;
> On 2014/09/15 22:35:51, Vitaly Pavlenko wrote:
> > Hi,
> > 
> > I'm compiling Chrome JS with Closure Compiler. Property |controlledBy| is of
> > type (string|null) here, but we trying to remove null values from properties
> > defined by cr.defineProperty(). So I tried to rewrite this line with
> > 
> > this.controlledBy = '';
> > 
> > However, that didn't work and caused a test failure:
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...
> > 
> > I don't see any connection between this variable and test failure. 
> > 
> > So if you remember this code, please help me with to answer either question:
> > 
> > a) Is it possible to use empty string for |controlledBy| instead of null?
What
> > should I rewrite besides this file?
> > 
> > b) Could you suggest, how is that test failure connected with my change?
> > 
> > Thank you,
> > Vitaly
> 
> |controlledBy| controls two things:
> 
> 1) If the property is non-null, a certain UI element (the indicator) will be
> shown.
> 2) The actual property value determines how the indicator behaves.
> 
> Your code is triggering 1), which makes the test fail. You can change the code
> to treat '' like it treats null now.
> 
> However, note that the |controlledBy| value is copied from an event and it may
> be null there. If you want to find all the places that fire this event, look
for
> the comment "Create a synthetic pref change event decorated as
> CoreOptionsHandler::CreateValueForPref() does." I marked all the places in the
> code that fire such events in this way.

Could you please link the code where the control of 1) is going? I failed to
found such a place: everywhere I looked at by searching |controlledBy|
comparison can't tell apart null and ''.

[bartfab (slow), 2014-09-17 08:18:37]

https://chromiumcodereview.appspot.com/11066015/diff/1008/chrome/browser/reso...
File chrome/browser/resources/options/controlled_setting.js (right):

https://chromiumcodereview.appspot.com/11066015/diff/1008/chrome/browser/reso...
chrome/browser/resources/options/controlled_setting.js:89: this.controlledBy =
null;
On 2014/09/16 14:26:08, bartfab wrote:
> On 2014/09/15 22:35:51, Vitaly Pavlenko wrote:
> > Hi,
> > 
> > I'm compiling Chrome JS with Closure Compiler. Property |controlledBy| is of
> > type (string|null) here, but we trying to remove null values from properties
> > defined by cr.defineProperty(). So I tried to rewrite this line with
> > 
> > this.controlledBy = '';
> > 
> > However, that didn't work and caused a test failure:
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...
> > 
> > I don't see any connection between this variable and test failure. 
> > 
> > So if you remember this code, please help me with to answer either question:
> > 
> > a) Is it possible to use empty string for |controlledBy| instead of null?
What
> > should I rewrite besides this file?
> > 
> > b) Could you suggest, how is that test failure connected with my change?
> > 
> > Thank you,
> > Vitaly
> 
> |controlledBy| controls two things:
> 
> 1) If the property is non-null, a certain UI element (the indicator) will be
> shown.
> 2) The actual property value determines how the indicator behaves.
> 
> Your code is triggering 1), which makes the test fail. You can change the code
> to treat '' like it treats null now.
> 
> However, note that the |controlledBy| value is copied from an event and it may
> be null there. If you want to find all the places that fire this event, look
for
> the comment "Create a synthetic pref change event decorated as
> CoreOptionsHandler::CreateValueForPref() does." I marked all the places in the
> code that fire such events in this way.

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...

This rule will be triggered by '' but not by null.