MD Settings: cr-slider: Make display consistent and clean up.

chrome/browser/resources/settings/device_page/display.js

Index: chrome/browser/resources/settings/device_page/display.js
diff --git a/chrome/browser/resources/settings/device_page/display.js b/chrome/browser/resources/settings/device_page/display.js
index 6ad553b9eac1358ff4c46fde61b98a0fd1521a5e..3307314f33bd0e572571682ade4b071b2c99cd34 100644
--- a/chrome/browser/resources/settings/device_page/display.js
+++ b/chrome/browser/resources/settings/device_page/display.js
@@ -53,14 +53,14 @@ Polymer({
       notify: true,
     },
 
-    /** Maximum mode index value for slider. */
-    maxModeIndex_: {type: Number, value: 0},
+    /** @private {!Array<number>} Mode index values for slider. */

[dschuyler, 2016/12/22 02:36:35]

This looks like a departure from the common style, which would be:

/**
 * Mode index values for slider. 
 * @private {!Array<number>}
 */

grepping in settings/ found 1323 hits for @private. Only one of those, in
passwords_and_forms_page.js uses the style on line 56 :)

If you do want to change the way we are commenting @private, I'm not against
that. I just thought this might be a whoops or "didn't notice that" kind of
thing.

[stevenjb, 2016/12/22 02:55:25]

It's shorthand, used more commonly when the type isn't needed, but as you
noticed not entirely without precedent. I generally prefer short when it's
clear.

[michaelpg, 2016/12/22 07:31:32]

We must be searching differently, because I found a settings hit (only) in
keyboard.js:

\/\*\*\s@private\s{.*}\s[^*]
+1. I'm surprised we have so little of this; I thought I used it commonly myself
but I guess not? Also a small amount of precedent for the substantially similar
@type annotation:

/** @type {foo} Foo fighters. */

+    modeValues_: Array,
 
-    /** Selected mode index value for slider. */
-    selectedModeIndex_: {type: Number},
+    /** @private Selected mode index value for slider. */
+    selectedModeIndex_: Number,
 
-    /** Immediate selected mode index value for slider. */
-    immediateSelectedModeIndex_: {type: Number, value: 0}
+    /** @private Selected mode index received from chrome. */
+    currentSelectedModeIndex_: Number,
   },
 
   /**
@@ -150,15 +150,16 @@ Polymer({
 
   /** @private */
   selectedDisplayChanged_: function() {
-    // Set maxModeIndex first so that the slider updates correctly.
-    if (this.selectedDisplay.modes.length == 0) {
-      this.maxModeIndex_ = 0;
+    // Set |modeValues_| before |selectedModeIndex_| so that the slider updates
+    // correctly.
+    let numModes = this.selectedDisplay.modes.length;
+    if (numModes == 0) {
+      this.modeValues_ = [];
       this.selectedModeIndex_ = 0;
       return;
     }
-    this.maxModeIndex_ = this.selectedDisplay.modes.length - 1;
+    this.modeValues_ = Array.from(Array(numModes).keys());

[michaelpg, 2016/12/22 07:31:32]

I think I have a TODO somewhere to add this to cr-slider directly so we can just
say "use values as ticks" instead of doing all this crap. Not that we have to
fix it now.

[stevenjb, 2016/12/22 20:42:18]

You do, and I thought about doing it that way, but in order for it to be robust
it has to be kind of complicated (e.g. we have one slider that starts at 0 and
one that starts at 1, so it needs min and max). Given how few sliders we have
(and this one may go away), it didn't seem worth the bother.

     this.selectedModeIndex_ = this.getSelectedModeIndex_(this.selectedDisplay);
-    this.immediateSelectedModeIndex_ = this.selectedModeIndex_;
   },
 
   /**
@@ -249,22 +250,15 @@ Polymer({
    */
   getResolutionText_: function() {
     if (this.selectedDisplay.modes.length == 0) {
-      var widthStr = this.selectedDisplay.bounds.width.toString();
-      var heightStr = this.selectedDisplay.bounds.height.toString();
+      let widthStr = this.selectedDisplay.bounds.width.toString();
+      let heightStr = this.selectedDisplay.bounds.height.toString();
       return this.i18n('displayResolutionText', widthStr, heightStr);
     }
-    // immediateSelectedModeIndex_ is bound to paper-slider.immediate-value
-    // which may not be valid, or may not have updated yet when this is called.
-    if (isNaN(this.immediateSelectedModeIndex_) ||
-        this.immediateSelectedModeIndex_ >= this.selectedDisplay.modes.length) {
-      this.immediateSelectedModeIndex_ =
-          this.getSelectedModeIndex_(this.selectedDisplay);
-    }
-    var mode = this.selectedDisplay.modes[this.immediateSelectedModeIndex_];
-    var best =
+    let mode = this.selectedDisplay.modes[this.selectedModeIndex_];
+    let best =
         this.selectedDisplay.isInternal ? mode.uiScale == 1.0 : mode.isNative;
-    var widthStr = mode.width.toString();
-    var heightStr = mode.height.toString();
+    let widthStr = mode.width.toString();
+    let heightStr = mode.height.toString();
     if (best)
       return this.i18n('displayResolutionTextBest', widthStr, heightStr);
     else if (mode.isNative)
@@ -317,18 +311,20 @@ Polymer({
   },
 
   /**
-   * @param {!{target: !PaperSliderElement}} e
+   * Triggered when the 'change' event for the selected mode slider is
+   * triggered. This only occurs when the value is comitted (i.e. not while
+   * the slider is being dragged).
    * @private
    */
-  onChangeMode_: function(e) {
-    var curIndex = this.selectedModeIndex_;
-    var newIndex = parseInt(e.target.value, 10);
-    if (newIndex == curIndex)
+  onSelectedModeChange_: function() {
+    if (this.currentSelectedModeIndex_ === undefined ||

[michaelpg, 2016/12/22 07:31:32]

since we remove the listener in detached(), do we need to also clear
this.currentSelectedModeIndex_ there for this logic to still work after another
attach()?

[stevenjb, 2016/12/22 20:42:18]

That's probably a good idea. Done.

+        this.currentSelectedModeIndex_ == this.selectedModeIndex_) {
+      // Don't change the selected display mode until we have received an update
+      // from Chrome and the mode differs from the current mode.
       return;
-    assert(newIndex >= 0);
-    assert(newIndex < this.selectedDisplay.modes.length);
+    }
     /** @type {!chrome.system.display.DisplayProperties} */ var properties = {
-      displayMode: this.selectedDisplay.modes[newIndex]
+      displayMode: this.selectedDisplay.modes[this.selectedModeIndex_]
     };
     settings.display.systemDisplayApi.setDisplayProperties(
         this.selectedDisplay.id, properties,
@@ -398,6 +394,11 @@ Polymer({
     this.primaryDisplayId = (primaryDisplay && primaryDisplay.id) || '';
     this.selectedDisplay = selectedDisplay || primaryDisplay ||
         (this.displays && this.displays[0]);
+    // Save the selected mode index received from Chrome so that we do not
+    // send an unnecessary setDisplayProperties call (which would log an error).
+    this.currentSelectedModeIndex_ =
+        this.getSelectedModeIndex_(this.selectedDisplay);
+
     this.$.displayLayout.updateDisplays(this.displays, this.layouts);
   },