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..77cc9c173631626d4584ccabbcdc2bd2644e0c68 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_;
       }
 
@@ -44,7 +40,8 @@ cr.define('options', function() {
 
     /**
      * The given handler will be called when the user clicks on the 'reset to
-     * recommended value' link shown in the indicator bubble.
+     * recommended value' link shown in the indicator bubble. The |this| object
+     * will be the indicator itself.
      * @param {function()} handler The handler to be called.
      */
     set resetHandler(handler) {
@@ -71,7 +68,26 @@ 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 (event.value.controlledBy) {
+        this.controlledBy =
+            !this.value || String(event.value.value) == this.value ?
+            event.value.controlledBy : null;
+      } else if (event.value.recommendedValue != undefined) {
+        this.controlledBy =
+            !this.value || String(event.value.recommendedValue) == this.value ?
+            'hasRecommendation' : null;
+      } else {
+        this.controlledBy = null;

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

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]

|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.

[bartfab (slow), 2014/09/17 08:18:36]

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

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

+      }
     },
 
     /**
@@ -120,14 +136,16 @@ cr.define('options', function() {
       } else {
         var self = this;
 
-        // Work out the popup text.
+        // Construct the bubble text.
         defaultStrings = {
           'policy': loadTimeData.getString('controlledSettingPolicy'),
           'extension': loadTimeData.getString('controlledSettingExtension'),
           'recommended': loadTimeData.getString('controlledSettingRecommended'),
+          'hasRecommendation':
+              loadTimeData.getString('controlledSettingHasRecommendation'),
         };
 
-        // No controller, no popup.
+        // No controller, no bubble.
         if (!this.controlledBy || !(this.controlledBy in defaultStrings))
           return;
 
@@ -143,13 +161,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 +199,25 @@ 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 value of the associated preference that the indicator represents. If
+   * this is not set, the indicator will be visible whenever any value is
+   * enforced or recommended. If it is set, the indicator will be visible only
+   * when the enforced or recommended value matches the value it represents.
+   * This allows multiple indicators to be created for a set of radio buttons,
+   * ensuring that only one of them is visible at a time.
+   */
+  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',