Add scale to settings-slider

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

 // Copyright 2016 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.
 
 /**
  * @fileoverview
  * settings-slider wraps a paper-slider. It maps the slider's values from a
  * linear UI range to a range of real values.  When |value| does not map exactly
  * to a tick mark, it interpolates to the nearest tick.
  *
  * Unlike paper-slider, there is no distinction between value and
  * immediateValue; when either changes, the |value| property is updated.
  */
 Polymer({
   is: 'settings-slider',
 
   behaviors: [CrPolicyPrefBehavior],
 
   properties: {
     /** @type {!chrome.settingsPrivate.PrefObject} */
     pref: Object,
 
     /** @type {!Array<number>} Values corresponding to each tick. */
     tickValues: {type: Array, value: []},
 
+    /**
+     * |scale| is applied to the slider value if |tickValues| is empty,

[michaelpg, 2017/05/15 19:53:18]

the pref value is being scaled, not the slider value. |scale| is applied to the
pref value, the inverse of |scale| is applied to the slider value :-\

[stevenjb, 2017/05/15 21:45:09]

No, it's the other way around, but I will clarify the comment :)

[michaelpg, 2017/05/15 22:14:32]

it's still confusing -- I think we're reading it in opposite ways which are both
valid.

I'm thinking in terms of "The slider value is divided" (to calculate the new
pref value), or "The pref value will be multiplied by |scale|" (to determine the
slider value). You're thinking in terms of "The slider value will be the result
of the pref value multiplied by |scale|."

Can I suggest avoiding the confusing English bits?

 * The slider UI will reflect |pref.value| * |scale|.
 * When the slider changes, the pref will be set to |slider.value| / |scale|.

[stevenjb, 2017/05/15 22:24:00]

Dude, the code says:

slider.value = pref.value * scale.

I don't know how that can be more clear - the slider value, i.e. slider.value,
is multiplied by scale. I'll change the comment to use slider.value I guess.

[michaelpg, 2017/05/15 22:55:05]

What term is multiplied by scale in this equation?
The slider value is not multiplied by scale. Not being pedantic here, it's just
not.

If you have a pref value of 2 and a scale of 10, 2 is multiplied by 10 to create
the slider value, 20. If you were to multiply the slider value, i.e. 20, by
scale, you'd get 200. That's not what the code is doing.

+     * otherwise it is ignored. Can be used to set fractional pref values
+     * (paper-slider will not set fractional values).
+     */
+    scale: Number,
+
     min: Number,
 
     max: Number,
 
     labelMin: String,
 
     labelMax: String,
 
     disabled: Boolean,
 
@@ skipping 14 matching lines @@
    * @private
    */
   onSliderChanged_: function() {
     var sliderValue = isNaN(this.$.slider.immediateValue)
                          ? this.$.slider.value
                          : this.$.slider.immediateValue;
 
     var newValue;
     if (this.tickValues && this.tickValues.length > 0)
       newValue = this.tickValues[sliderValue];
+    else if (this.scale)
+      newValue = sliderValue / this.scale;

[afakhry, 2017/05/12 19:12:13]

Is it possible to give |scale| a default value of 1 and do this scaling all the
time?

[stevenjb, 2017/05/12 20:14:13]

It is, and I considered that, but this seemed slightly better to me since we do
not apply it when tickValues is defined. That said, I could be convinced either
way.

[afakhry, 2017/05/12 23:56:11]

I'm fine with either way.

[michaelpg, 2017/05/15 19:53:18]

Either way, I'd suggest an assert so it breaks if it's used incorrectly (e.g. if
scale is set check that tickValues is unset or vice versa)

[stevenjb, 2017/05/15 21:45:09]

Done (assert placed in valueChanged where it is easier to add and called less
frequently).

     else
       newValue = sliderValue;
 
     this.set('pref.value', newValue);
   },
 
   /** @private */
   computeDisableSlider_: function() {
     return this.disabled || this.isPrefEnforced();
   },
 
   /**
    * Updates the knob position when |pref.value| changes. If the knob is still
    * being dragged, this instead forces |pref.value| back to the current
    * position.
    * @private
    */
   valueChanged_: function() {
     // If |tickValues| is empty, simply set current value to the slider.
     if (this.tickValues.length == 0) {
-      this.$.slider.value = this.pref.value;
+      if (this.scale) {

[michaelpg, 2017/05/15 19:53:18]

in the same vein as afakhry's chomment, this may be easier to follow/maintain if
|scale| is applied as an operation instead of setting the slider inside each
branch.

var val = /** {number} */(this.pref.value);
if (this.scale) val *= this.scale;
this.$.slider.value = val;

[stevenjb, 2017/05/15 21:45:09]

Set default scale to 1.

+        this.$.slider.value =
+            /** @type {number} */ (this.pref.value) * this.scale;
+      } else {
+        this.$.slider.value = this.pref.value;
+      }
       return;
     }
 
     // First update the slider settings if |tickValues| was set.
     var numTicks = Math.max(1, this.tickValues.length);
     this.$.slider.max = numTicks - 1;
     // Limit the number of ticks to 10 to keep the slider from looking too busy.
     /** @const */ var MAX_TICKS = 10;
     this.$.slider.snaps = numTicks < MAX_TICKS;
     this.$.slider.maxMarkers = numTicks < MAX_TICKS ? numTicks : 0;
@@ skipping 39 matching lines @@
       if (difference < minDifference) {
         closestIndex = i;
         minDifference = difference;
       }
     }
 
     assert(typeof closestIndex != 'undefined');
     return closestIndex;
   },
 });