Finish implementation of controlled setting indicator

chrome/browser/resources/options/controlled_setting.js

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 cr.define('options', function() {
   var Preferences = options.Preferences;
 
   /**
    * A controlled setting indicator that can be placed on a setting as an
    * indicator that the value is controlled by some external entity such as
    * policy or an extension.
    * @constructor
    * @extends {HTMLSpanElement}
    */
   var ControlledSettingIndicator = cr.ui.define('span');
 
   ControlledSettingIndicator.prototype = {
     __proto__: HTMLSpanElement.prototype,
 
     /**
      * Decorates the base element to show the proper icon.
      */
     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_;
       }
 
       this.tabIndex = 0;
       this.setAttribute('role', 'button');
       this.addEventListener('click', this);
       this.addEventListener('keydown', this);
       this.addEventListener('mousedown', this);
     },
 
     /**
      * 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) {
       this.resetHandler_ = handler;
     },
 
     /**
      * Whether the indicator is currently showing a bubble.
      * @type {boolean}
      */
     get showingBubble() {
       return !!this.showingBubble_;
     },
     set showingBubble(showing) {
       if (showing)
         this.classList.add('showing-bubble');
       else
         this.classList.remove('showing-bubble');
       this.showingBubble_ = showing;
     },
 
     /**
      * Clears the preference associated with this indicator.
      * @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.

+      }
     },
 
     /**
      * Handle mouse and keyboard events, allowing the user to open and close a
      * bubble with further information.
      * @param {Event} event Mouse or keyboard event.
      */
     handleEvent: function(event) {
       switch (event.type) {
         // Toggle the bubble on left click. Let any other clicks propagate.
@@ skipping 28 matching lines @@
     /**
      * Open or close a bubble with further information about the pref.
      * @private
      */
     toggleBubble_: function() {
       if (this.showingBubble) {
         OptionsPage.hideBubble();
       } 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;
 
         var text = defaultStrings[this.controlledBy];
 
         // Apply text overrides.
         if (this.hasAttribute('text' + this.controlledBy))
           text = this.getAttribute('text' + this.controlledBy);
 
         // Create the DOM tree.
         var content = document.createElement('div');
         content.className = 'controlled-setting-bubble-content';
         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_();
           });
           container.appendChild(action);
           content.appendChild(container);
         }
 
         OptionsPage.showBubble(content, this);
       }
     },
@@ skipping 10 matching lines @@
    * associated preference take effect in the settings UI immediately but are
    * only actually committed when the user confirms the dialog. If the user
    * cancels the dialog instead, the changes are rolled back in the settings UI
    * and never committed.
    * @type {boolean}
    */
   cr.defineProperty(ControlledSettingIndicator, 'dialogPref',
                     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',
                     cr.PropertyKind.ATTR);
 
   // Export.
   return {
     ControlledSettingIndicator: ControlledSettingIndicator
   };
 });