Finish implementation of controlled setting indicator

chrome/browser/resources/options/controlled_setting.js

Index: chrome/browser/resources/options/controlled_setting.js
diff --git a/chrome/browser/resources/options/controlled_setting.js b/chrome/browser/resources/options/controlled_setting.js
index aa561007f0efdf4664c361eaa76b8f89bfc7f58f..68da1ea9c0ca9afa51af57ca731088144699373d 100644
--- a/chrome/browser/resources/options/controlled_setting.js
+++ b/chrome/browser/resources/options/controlled_setting.js
@@ -23,15 +23,11 @@ cr.define('options', function() {
     decorate: function() {
       var self = this;
 
-      // If there is a pref, track its controlledBy property in order to be able
-      // to bring up the correct bubble.
+      // If there is a pref, track its controlledBy and recommendedValue
+      // properties in order to be able to bring up the correct bubble.
       if (this.pref) {
-        Preferences.getInstance().addEventListener(this.pref,
-            function(event) {
-              var controlledBy = event.value.controlledBy;
-              self.controlledBy = controlledBy ? controlledBy : null;
-              OptionsPage.hideBubble();
-            });
+        Preferences.getInstance().addEventListener(
+            this.pref, this.handlePrefChange.bind(this));
         this.resetHandler = this.clearAssociatedPref_;
       }
 
@@ -71,7 +67,34 @@ cr.define('options', function() {
      * @private
      */
     clearAssociatedPref_: function() {
-      Preferences.clearPref(this.pref, this.dialogPref);
+      Preferences.clearPref(this.pref, !this.dialogPref);
+    },
+
+    /* Handle changes to the associated pref by hiding any currently visible
+     * bubble and updating the controlledBy property.
+     * @param {Event} event Pref change event.
+     */
+    handlePrefChange: function(event) {
+      OptionsPage.hideBubble();
+      if (this.type == 'radio') {
+        if (event.value.controlledBy) {
+          this.controlledBy = String(event.value.value) == this.value ?
+              event.value.controlledBy : null;
+        } else if (event.value.recommendedValue != undefined) {
+          this.controlledBy =
+              String(event.value.recommendedValue) == this.value ?
+              'hasRecommendation' : null;
+        } else {
+          this.controlledBy = null;
+        }
+      } else {
+        if (event.value.controlledBy)
+          this.controlledBy = event.value.controlledBy;
+        else if (event.value.recommendedValue != undefined)
+          this.controlledBy = 'hasRecommendation';
+        else
+          this.controlledBy = null;
+      }
     },
 
     /**
@@ -120,11 +143,13 @@ cr.define('options', function() {
       } else {
         var self = this;
 
-        // Work out the popup text.
+        // Work out the bubble text.

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

Can we change the wording from "Work out" to, say, "Construct"?

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

Done.

         defaultStrings = {
           'policy': loadTimeData.getString('controlledSettingPolicy'),
           'extension': loadTimeData.getString('controlledSettingExtension'),
           'recommended': loadTimeData.getString('controlledSettingRecommended'),
+          'hasRecommendation':
+              loadTimeData.getString('controlledSettingHasRecommendation'),
         };
 
         // No controller, no popup.

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

Nit: bubble

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

Done.

@@ -143,13 +168,14 @@ cr.define('options', function() {
         content.setAttribute('controlled-by', this.controlledBy);
         content.textContent = text;
 
-        if (this.controlledBy == 'recommended' && this.resetHandler_) {
+        if (this.controlledBy == 'hasRecommendation' && this.resetHandler_ &&
+            !this.readOnly) {
           var container = document.createElement('div');
           var action = document.createElement('button');
           action.classList.add('link-button');
           action.classList.add('controlled-setting-bubble-action');
           action.textContent =
-              loadTimeData.getString('controlledSettingApplyRecommendation');
+              loadTimeData.getString('controlledSettingFollowRecommendation');
           action.addEventListener('click', function(event) {
             self.resetHandler_();
           });
@@ -180,8 +206,29 @@ cr.define('options', function() {
                     cr.PropertyKind.BOOL_ATTR);
 
   /**
-   * Whether the associated preference is controlled by a source other than the
-   * user's setting (can be 'policy', 'extension', 'recommended' or unset).
+   * The kind of input element that this indicator refers to.

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

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]

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:11]

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.

+   * @type {string}
+   */
+  cr.defineProperty(ControlledSettingIndicator, 'type',
+                    cr.PropertyKind.ATTR);
+
+  /**
+   * If this indicator refers to a radio button, the value of the associated
+   * preference that the radio button represents.
+   * @type {string}
+   */
+  cr.defineProperty(ControlledSettingIndicator, 'value',
+                    cr.PropertyKind.ATTR);
+
+  /**
+   * The status of the associated preference:
+   * - 'policy':            A specific value is enfoced by policy.
+   * - 'extension':         A specific value is enforced by an extension.
+   * - 'recommended':       A value is recommended by policy. The user could
+   *                        override this recommendation but has not done so.
+   * - 'hasRecommendation': A value is recommended by policy. The user has
+   *                        overridden this recommendation.
+   * - unset:               The value is controlled by the user alone.
    * @type {string}
    */
   cr.defineProperty(ControlledSettingIndicator, 'controlledBy',