[MD settings] adding closure compile for settings dropdown menu (and cleaning up)

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

 // Copyright 2015 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.
 
 /**
+ * This tuple is made up of a (value, name, attribute).  The value and name are
+ * used by the dropdown menu.  The attribute is optional 'user data' that is
+ * ignored by the dropdown menu.
+ * @typedef {{
+ *   0: (number|string),
+ *   1: string,
+ *   2: (string|undefined)
+ * }}
+ */
+var DropdownMenuOption;
+
+/**
+ * @typedef {!Array<DropdownMenuOption>}
+ */
+var DropdownMenuOptionList;
+
+/**
  * 'settings-dropdown-menu' is a control for displaying options
  * in the settings.

[michaelpg, 2015/11/12 23:33:05]

add a sentence explaining the not-found item behavior

  *
  * Example:
  *
  *   <settings-dropdown-menu pref="{{prefs.foo}}">
  *   </settings-dropdown-menu>
  *
  * @group Chrome Settings Elements
  * @element settings-dropdown-menu
  */
 Polymer({
   is: 'settings-dropdown-menu',
 
   properties: {
     /**
      * A text label for the drop-down menu.
      */
     label: {
       type: String,
     },
 
     /**
      * List of options for the drop-down menu.
-     * @type {!Array<{0: (Object|number|string), 1: string,
-     *   2: (string|undefined)}>}
+     * @type {!DropdownMenuOptionList}
      */
     menuOptions: {
-      notify: true,
       type: Array,
       value: function() { return []; },
     },
 
     /**
      * A single Preference object being tracked.
-     * @type {?PrefObject}
      */
     pref: {
       type: Object,
       notify: true,
     },
 
     /**
      * Either loading text or the label for the drop-down menu.
      * @private
      */
@@ skipping 33 matching lines @@
     /**
      * Whether to show the 'custom' item.
      * @private
      */
     showNotFoundValue_: {
       type: Boolean,
     },
   },
 
   behaviors: [
-    I18nBehavior
+    I18nBehavior,
   ],
 
   observers: [
     'checkSetup_(menuOptions, selectedValue_)',
     'prefChanged_(pref.value)',
   ],
 
   /**
    * 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 {!DropdownMenuOptionList} menuOptions
    * @param {string} selectedValue_
    * @private
    */
   checkSetup_: function(menuOptions, selectedValue_) {
     if (!this.menuOptions.length) {
       return;
     }
 
     if (!Object.keys(this.menuMap_).length) {
       // Create a map from index value [0] back to the index i.
       var result = {};
       for (var i = 0; i < this.menuOptions.length; ++i)
-        result[JSON.stringify(this.menuOptions[i][0])] = i.toString();
+        result[String(this.menuOptions[i][0])] = i.toString();

[michaelpg, 2015/11/12 23:33:05]

btw, I think foo.toString() is more idiomatic than String(foo). I was being dumb
when I suggested String(foo).

       this.menuMap_ = result;
     }
 
     // We need the menuOptions and the selectedValue_.  They may arrive
     // at different times (each is asynchronous).
-    this.selected_ = this.getItemIndex(this.selectedValue_);
+    this.selected_ = this.itemIndex_(this.selectedValue_);
     this.menuLabel_ = this.label;
     this.$.dropdownMenu.disabled = false;

[michaelpg, 2015/11/12 23:33:05]

Also, what if I want a <settings-dropdown-menu disabled>? (that property will
probably have to be added soon or later, so we shouldn't override its effects)

[michaelpg, 2015/11/12 23:33:05]

I think disabling the control until the items are available is unnecessary and
will only exacerbate any FOUC. Unless I'm misunderstanding the purpose of this.

   },
 
   /**
    * @param {string} item A value from the menuOptions array.
    * @return {string}
    * @private
    */
-  getItemIndex: function(item) {
+  itemIndex_: function(item) {
     var result = this.menuMap_[item];
     if (result)
       return result;
     this.showNotFoundValue_ = true;

[michaelpg, 2015/11/12 23:33:05]

same note from the landed CL: a getter shouldn't add an item to the array, and
this looks like a getter.

     // The 'not found' item is added as the last of the options.
     return (this.menuOptions.length).toString();

[michaelpg, 2015/11/12 23:33:05]

no need for associative parens

   },
 
   /**
    * @param {string} index An index into the menuOptions array.
-   * @return {Object|number|string|undefined}
+   * @return {number|string|undefined}
    * @private
    */
-  getItemValue: function(index) {
+  itemValue_: function(index) {
     if (this.menuOptions.length) {
       var result = this.menuOptions[index];

[michaelpg, 2015/11/12 23:33:05]

don't index into an array with a string

       if (result)
         return result[0];
     }
     return undefined;
   },
 
   /**
    * Pass the selection change to the pref value.
    * @private
    */
   onSelectedChanged_: function() {
-    var prefValue = this.getItemValue(this.selected_);
+    var prefValue = this.itemValue_(this.selected_);
     if (prefValue !== undefined) {
-      this.selectedValue_ = JSON.stringify(prefValue);
+      this.selectedValue_ = String(prefValue);

[michaelpg, 2015/11/12 23:33:05]

toString

       this.set('pref.value', prefValue);
     }
   },
 
   /**
    * @param {number|string} value A value from the menuOptions array.
    * @private
    */
   prefChanged_: function(value) {
-    this.selectedValue_ = JSON.stringify(value);
+    if (value !== undefined)
+      this.selectedValue_ = String(value);

[michaelpg, 2015/11/12 23:33:05]

toString

   },
 });