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.
      * @param {function()} handler The handler to be called.
      */
     set resetHandler(handler) {

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

Please make it clear in the comment with which 'this' object the handler is
called.

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

Done.

       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 (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;
+      }
     },
 
     /**
      * 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.
+        // 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.

         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.

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

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.

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

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

         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 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',
                     cr.PropertyKind.ATTR);
 
   // Export.
   return {
     ControlledSettingIndicator: ControlledSettingIndicator
   };
 });