MD Settings: handle zoom as floats to fix ghost zoom level issue

chrome/browser/resources/settings/appearance_page/appearance_page.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.
 
 /**
  * 'settings-appearance-page' is the settings page containing appearance
  * settings.
  *
  * Example:
  *
@@ skipping 36 matching lines @@
           {value: 12, name: loadTimeData.getString('small')},
           {value: 16, name: loadTimeData.getString('medium')},
           {value: 20, name: loadTimeData.getString('large')},
           {value: 24, name: loadTimeData.getString('veryLarge')},
         ];
       },
     },
 
     /**
      * List of options for the page zoom drop-down menu.
      * @type {!DropdownMenuOptionList}

[dpapad, 2016/10/15 00:58:07]

Type annotation no longer accurate. Should be !Array<number>

[Dan Beam, 2016/10/17 19:20:26]

Done.

      */
-    pageZoomOptions_: {
+    pageZoomLevels_: {
       readOnly: true,
       type: Array,
       value: [
-        {value: 25, name: '25%'},
-        {value: 33, name: '33%'},
-        {value: 50, name: '50%'},
-        {value: 67, name: '67%'},
-        {value: 75, name: '75%'},
-        {value: 80, name: '80%'},
-        {value: 90, name: '90%'},
-        {value: 100, name: '100%'},
-        {value: 110, name: '110%'},
-        {value: 125, name: '125%'},
-        {value: 150, name: '150%'},
-        {value: 175, name: '175%'},
-        {value: 200, name: '200%'},
-        {value: 250, name: '250%'},
-        {value: 300, name: '300%'},
-        {value: 400, name: '400%'},
-        {value: 500, name: '500%'},
+        // TODO(dbeam): get these dynamically from C++ instead.
+        1 / 4,
+        1 / 3,

[michaelpg, 2016/10/17 08:46:08]

when it comes to repeating decimals, is this safe?

JSON pref store => double => JavaScript number => <option> string => JavaScript
number => double => JSON pref store

[Dan Beam, 2016/10/17 19:20:26]

safe enough, yes
https://cs.chromium.org/chromium/src/content/common/page_zoom.cc?l=16

[michaelpg, 2016/10/17 19:58:15]

ok, that's good. what about the JS side? we populate the <select> with option
values generated in JS (implicitly converted to strings), and set the value with
a Number coming from C++ (implicitly converted to a string).

i guess... is Number.prototype.toString guaranteed to return the same string for
two equivalent rational numbers, one produced in JS and one produced in C++ and
sent to JS via some binding? is the answer implementation or architecture
dependent?

on my current machine, (2/3).toString() isn't accurate: "0.6666666666666666" --
even though with the same number of digits I can safely represent
0.6666666666666667 in a variable that prints the 7.

could CallJavascriptFunction('foo', FundamentalValue(2.0/3)) call foo with an
argument like 0.6666666666666667, resulting in the select not finding a correct
option?

tl;dr using arbitrary real numbers converted to strings as keys scares me

[Dan Beam, 2016/10/17 21:01:20]

i understand your concerns.

i think getting these numbers from zoom::kPresetZoomFactors in C++ and
DCHECK()ing that ZoomValuesEqual() one of them when setting from JS is best way
to go here.

+        1 / 2,
+        2 / 3,
+        3 / 4,
+        4 / 5,
+        9 / 10,
+        1,
+        11 / 10,
+        5 / 4,
+        3 / 2,
+        7 / 4,
+        2,
+        5 / 2,
+        3,
+        4,
+        5,
       ],
     },
 
     /** @private */
     themeSublabel_: String,
 
     /**
      * Dictionary defining page visibility.
      * @type {!AppearancePageVisibility}
      */
@@ skipping 21 matching lines @@
     // TODO(dschuyler): Look into adding a listener for the
     // default zoom percent.
     chrome.settingsPrivate.getDefaultZoomPercent(function(value) {
       // TODO(dpapad): Non-integer values will cause no <option> to be selected
       // until crbug.com/655742 is addressed.
       this.$.zoomLevel.value = value;
     }.bind(this));
   },
 
   /**
+   * @param {number} zoom
+   * @return {number} A zoom easier read by users.

[dpapad, 2016/10/15 00:58:07]

@private

[michaelpg, 2016/10/17 08:46:08]

{string} maybe

[Dan Beam, 2016/10/17 19:20:26]

Done.

[Dan Beam, 2016/10/17 19:20:26]

it's not a string

+   */
+  formatZoom_: function(zoom) {
+    return Math.round(zoom * 100);

[michaelpg, 2016/10/17 08:46:08]

return (zoom * 100).toFixed();

[Dan Beam, 2016/10/17 19:20:26]

what's the functional difference?  especially for the small set we're operating
on?

[michaelpg, 2016/10/17 19:58:14]

toFixed returns a string. it's mathematically equivalent, but if we do return a
number I think "format" is the wrong name.

+  },
+
+  /**
    * @param {boolean} isNtp Whether to use the NTP as the home page.
    * @param {string} homepage If not using NTP, use this URL.
    * @return {string} The sub-label.
    * @private
    */
   getShowHomeSubLabel_: function(isNtp, homepage) {
     if (isNtp)
       return this.i18n('homePageNtp');
     return homepage || this.i18n('exampleDotCom');
   },
@@ skipping 94 matching lines @@
 
   /**
    * @param {boolean} bookmarksBarVisible if bookmarks bar option is visible.
    * @return {string} 'first' if the argument is false or empty otherwise.
    * @private
    */
   getFirst_: function(bookmarksBarVisible) {
     return !bookmarksBarVisible ? 'first' : '';
   }
 });