MD Settings: Simplify settings-dropdown-menu

chrome/browser/resources/settings/controls/settings_dropdown_menu.js

Index: chrome/browser/resources/settings/controls/settings_dropdown_menu.js
diff --git a/chrome/browser/resources/settings/controls/settings_dropdown_menu.js b/chrome/browser/resources/settings/controls/settings_dropdown_menu.js
index 28bddf25f5cd52809968372660c5846e359f8733..18e92b320d385689f0f357aa6972f1475ca55513 100644
--- a/chrome/browser/resources/settings/controls/settings_dropdown_menu.js
+++ b/chrome/browser/resources/settings/controls/settings_dropdown_menu.js
@@ -18,28 +18,20 @@ Polymer({
   is: 'settings-dropdown-menu',
 
   properties: {
-    /**
-     * A text label for the drop-down menu.
-     */
-    label: {
-      type: String,
-    },
+    /** A text label for the drop-down menu. */
+    label: String,
 
     /**
      * List of options for the drop-down menu.
      * @type {!Array<{0: (Object|number|string), 1: string,
-     *   2: (string|undefined)}>}
+     *     2: (string|undefined)}>}
      */
     menuOptions: {
-      notify: true,
       type: Array,
       value: function() { return []; },
     },
 
-    /**
-     * A single Preference object being tracked.
-     * @type {?PrefObject}

[stevenjb, 2015/11/11 02:04:21]

Can we type this (correctly)?

-     */
+    /** A single Preference object being tracked. */
     pref: {
       type: Object,
       notify: true,
@@ -54,7 +46,9 @@ Polymer({
       value: function() { return loadTimeData.getString('loading'); },
     },
 
-     /**
+    disabled: Boolean,

[stevenjb, 2015/11/11 02:04:21]

If we actually need this, let's just document that this reflects the HTML
element property.

[michaelpg, 2015/11/13 00:33:08]

This is just an HTMLElement so it doesn't have that property. (HTMLInputElements
have a disabled property.)

BTW, we should add reflectToAttribute: true and (if we end up disabling
automatically) *maybe* notify: true.

[michaelpg, 2015/11/17 05:08:55]

Done.

+
+    /**
      * A reverse lookup from the menu value back to the index in the

[michaelpg, 2015/11/13 00:33:08]

Crap! I forgot to update this doc. This is a reverse lookup from the menu value
(the stringified item[0] of each menuOption) to the original value (the item[0]
of each menuOption).

Think of it as a hash map from (menuOption[i][0].toString()) to
(menuOption[i][0]).

The type should be Object<string, (string|number|Object)>.

      * menuOptions array.
      * @private {!Object<string, string>}
@@ -65,113 +59,102 @@ Polymer({
     },
 
     /**
-     * The current selected item (an index number as a string).
+     * The current selected value, as a string.

[dschuyler, 2015/11/13 00:11:16]

Is this really the value?  Doesn't this get run through a lookup map to get the
value?

[michaelpg, 2015/11/13 00:33:08]

You're right, but that's why I kept "as a string".

The <paper-menu attr-for-selected="data-value"> has a bunch of <paper-items
data-value="some string">. So this matches the "selected" property of the
paper-menu. It comes from converting the values of the options in menuOptions to
strings via data binding to the data-value attribute.

[dschuyler, 2015/11/13 19:20:29]

Ah, thanks for the explanation.  
That data-value/attr-for-selected is rather nice.

I tinkered with it a bit, and it looks like the type 
that is initially set for |selected_| is ignored.
if the data-value is a number, then selected_ becomes 
a number; if data-value is a string, then selected_ 
becomes a string.  That seems even more handy since 
we can set the type in the menu-options.

[michaelpg, 2015/11/13 20:05:12]

Interesting! And weird and confusing. And apparently inconsistent. I filed
https://github.com/PolymerElements/iron-selector/issues/84 to track this.

      * @private
      */
-    selected_: {
-      notify: true,
-      observer: 'onSelectedChanged_',
-      type: String,
-    },
+    selected_: String,
 
     /**
-     * The current selected pref value.
+     * Whether to show the 'custom' item.
      * @private
      */
-    selectedValue_: {
-      type: String,
+    showNotFoundValue_: {
+      type: Boolean,
+      computed: 'isSelectedNotFound_(selected_)',
     },
 
     /**
-     * Whether to show the 'custom' item.
+     * The 'custom' item.
      * @private
      */
-    showNotFoundValue_: {
-      type: Boolean,
+    notFoundItem_: {
+      type: Array,
+      value: function() {
+        return ['', this.i18n('custom')];
+      },
     },
   },
 
   behaviors: [
-    I18nBehavior
+    I18nBehavior,
+    SelectablePrefBehavior,
   ],
 
   observers: [
-    'checkSetup_(menuOptions, selectedValue_)',
-    'prefChanged_(pref.value)',
+    'checkSetup_(menuOptions)',
+    'updateSelected_(pref.value)',
   ],
 
+  ready: function() {
+    this.checkSetup_(this.menuOptions);
+  },
+
   /**
    * Check to see if we have all the pieces needed to enable the control.
    * @param {!Array<{0: (Object|number|string), 1: string,
-   *   2: (string|undefined)}>} menuOptions
-   * @param {string} selectedValue_
+   *     2: (string|undefined)}>} menuOptions
    * @private
    */
-  checkSetup_: function(menuOptions, selectedValue_) {
-    if (!this.menuOptions.length) {
+  checkSetup_: function(menuOptions) {
+    if (!this.menuOptions.length || !this.menuMap_) {
       return;
     }
 
     if (!Object.keys(this.menuMap_).length) {
-      // Create a map from index value [0] back to the index i.
+      // Create a map from index value [0] to the original value.

[michaelpg, 2015/11/13 00:33:08]

This was ambiguous. Or just wrong. I should have said "from the stringified
value to the original value".

       var result = {};
-      for (var i = 0; i < this.menuOptions.length; ++i)
-        result[JSON.stringify(this.menuOptions[i][0])] = i.toString();
+      for (var option of this.menuOptions)
+        result[option[0].toString()] = option[0];
       this.menuMap_ = result;
+
+      this.menuLabel_ = this.label;
     }
 
-    // We need the menuOptions and the selectedValue_.  They may arrive
-    // at different times (each is asynchronous).
-    this.selected_ = this.getItemIndex(this.selectedValue_);
-    this.menuLabel_ = this.label;
-    this.$.dropdownMenu.disabled = false;
+    this.updateSelected_();
   },
 
   /**
-   * @param {string} item A value from the menuOptions array.
-   * @return {string}
-   * @private
-   */
-  getItemIndex: function(item) {
-    var result = this.menuMap_[item];
-    if (result)
-      return result;
-    this.showNotFoundValue_ = true;
-    // The 'not found' item is added as the last of the options.
-    return (this.menuOptions.length).toString();
-  },
-
-  /**
-   * @param {string} index An index into the menuOptions array.
-   * @return {Object|number|string|undefined}
+   * Pass the selection change to the pref value.
    * @private
    */
-  getItemValue: function(index) {
-    if (this.menuOptions.length) {
-      var result = this.menuOptions[index];
-      if (result)
-        return result[0];
-    }
-    return undefined;
+  onSelect_: function() {
+    var selected = this.menuMap_[this.selected_];

[dschuyler, 2015/11/13 00:11:16]

It is confusing that selected_ is translated into 
selected.  Err, well, I happen to already know it's translating
an index number into a preference value string, but it's 
hard to see that with lhs and rhs called selected{_}.

Maybe prefString on the lhs?

[michaelpg, 2015/11/13 00:33:08]

No, we're not using index numbers in this CL. We're using the data-value
attribute and taking advantage of the paper-menu's attrForSelected property so
we don't have to handle the indices manually.

The conversion here is done solely to get out the same type that came in via
menuOptions, since selected_ is always a String. And now I don't even think we
need to do that.

[dschuyler, 2015/11/13 19:20:29]

Now that I have a better understanding of the data-value, it looks like the 
menuMap_ may be unneeded.  Is that what you're saying may no longer be needed -
if so, I think you're right.

[michaelpg, 2015/11/13 20:05:12]

Yep.

+    if (selected == undefined)
+      return;
+    var prefValue = this.toPrefValue(selected);
+    if (prefValue !== undefined)
+      this.set('pref.value', prefValue);
   },
 
   /**
-   * Pass the selection change to the pref value.
+   * Updates the selected item when the pref or menuOptions change.
    * @private
    */
-  onSelectedChanged_: function() {
-    var prefValue = this.getItemValue(this.selected_);
-    if (prefValue !== undefined) {
-      this.selectedValue_ = JSON.stringify(prefValue);
-      this.set('pref.value', prefValue);
+  updateSelected_: function() {
+    var prefString = this.prefToString();
+    if (this.menuMap_[prefString] == undefined) {
+      this.selected_ = this.notFoundItem_[0];
+    } else {
+      this.selected_ = prefString;
     }
   },
 
   /**
-   * @param {number|string} value A value from the menuOptions array.
+   * @param {string} selected
+   * @return {boolean}
    * @private
    */
-  prefChanged_: function(value) {
-    this.selectedValue_ = JSON.stringify(value);
+  isSelectedNotFound_: function(selected) {
+    return selected == this.notFoundItem_[0];

[dschuyler, 2015/11/13 00:11:16]

Why is this using an array element rather than just using '' directly?

[michaelpg, 2015/11/13 00:33:08]

Meh. It seemed more natural to me to make notFoundItem_ the same type as
menuOption items, but it could just as easily be a const somewhere if that makes
more sense.

[dschuyler, 2015/11/13 19:20:29]

Maybe a closure type could be made for menu-option elements and this 
could be one of those types.  I think that would relay the intent.

[michaelpg, 2015/11/13 20:05:13]

SGTM

   },
 });