Add scale to settings-slider

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

Index: chrome/browser/resources/settings/controls/settings_slider.js
diff --git a/chrome/browser/resources/settings/controls/settings_slider.js b/chrome/browser/resources/settings/controls/settings_slider.js
index 1d7e2a94372ca4f788a7972bb1dc37e8c08cf372..fd121e3bbc6cbbece2b63443a8e2210d5185ba08 100644
--- a/chrome/browser/resources/settings/controls/settings_slider.js
+++ b/chrome/browser/resources/settings/controls/settings_slider.js
@@ -23,6 +23,13 @@ Polymer({
     /** @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,
@@ -57,6 +64,8 @@ Polymer({
     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;
 
@@ -77,7 +86,12 @@ Polymer({
   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;
     }