Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2422603003:
MD Settings: handle zoom as floats to fix ghost zoom level issue (Closed)
|
Created:
4 years, 2 months ago by Dan Beam Modified:
4 years, 2 months ago CC:
arv+watch_chromium.org, chromium-apps-reviews_chromium.org, chromium-reviews, dbeam+watch-settings_chromium.org, extensions-reviews_chromium.org, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, stevenjb+watch-md-settings_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD Settings: handle zoom as floats to fix ghost zoom level issue
R=dpapad@chromium.org
TBR=rdevlin.cronin@chromium.org
BUG=655742
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/0c1d68f185e5651164542cb6dd1c6ec9548ec1b1
Cr-Commit-Position: refs/heads/master@{#425569}
Patch Set 1 : todo #
Total comments: 15
Dependent Patchsets: Messages
Total messages: 27 (14 generated)
Description was changed from ========== MD Settings: handle zoom as floats to fix ghost zoom level issue R=dpapad@chromium.org BUG=655742 ========== to ========== MD Settings: handle zoom as floats to fix ghost zoom level issue R=dpapad@chromium.org BUG=655742 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Patchset #1 (id:1) has been deleted
sanity restored?
https://codereview.chromium.org/2422603003/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/appearance_page/appearance_page.js (right): https://codereview.chromium.org/2422603003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/appearance_page/appearance_page.js:57: * @type {!DropdownMenuOptionList} Type annotation no longer accurate. Should be !Array<number> https://codereview.chromium.org/2422603003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/appearance_page/appearance_page.js:123: * @return {number} A zoom easier read by users. @private
LGTM, given green tests.
The CQ bit was checked by dbeam@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by dbeam@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by dbeam@chromium.org
Description was changed from ========== MD Settings: handle zoom as floats to fix ghost zoom level issue R=dpapad@chromium.org BUG=655742 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: handle zoom as floats to fix ghost zoom level issue R=dpapad@chromium.org TBR=rdevlin.cronin@chromium.org BUG=655742 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
dbeam@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
TBR=rdevlin.cronin@ for long -> double
The CQ bit was checked by dbeam@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== MD Settings: handle zoom as floats to fix ghost zoom level issue R=dpapad@chromium.org TBR=rdevlin.cronin@chromium.org BUG=655742 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: handle zoom as floats to fix ghost zoom level issue R=dpapad@chromium.org TBR=rdevlin.cronin@chromium.org BUG=655742 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #1 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== MD Settings: handle zoom as floats to fix ghost zoom level issue R=dpapad@chromium.org TBR=rdevlin.cronin@chromium.org BUG=655742 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: handle zoom as floats to fix ghost zoom level issue R=dpapad@chromium.org TBR=rdevlin.cronin@chromium.org BUG=655742 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/0c1d68f185e5651164542cb6dd1c6ec9548ec1b1 Cr-Commit-Position: refs/heads/master@{#425569} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/0c1d68f185e5651164542cb6dd1c6ec9548ec1b1 Cr-Commit-Position: refs/heads/master@{#425569}
Message was sent while issue was closed.
michaelpg@chromium.org changed reviewers: + michaelpg@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2422603003/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/appearance_page/appearance_page.js (right): https://codereview.chromium.org/2422603003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/appearance_page/appearance_page.js:65: 1 / 3, when it comes to repeating decimals, is this safe? JSON pref store => double => JavaScript number => <option> string => JavaScript number => double => JSON pref store https://codereview.chromium.org/2422603003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/appearance_page/appearance_page.js:123: * @return {number} A zoom easier read by users. {string} maybe https://codereview.chromium.org/2422603003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/appearance_page/appearance_page.js:126: return Math.round(zoom * 100); return (zoom * 100).toFixed(); https://codereview.chromium.org/2422603003/diff/20001/chrome/common/extension... File chrome/common/extensions/api/settings_private.idl (right): https://codereview.chromium.org/2422603003/diff/20001/chrome/common/extension... chrome/common/extensions/api/settings_private.idl:82: static void setDefaultZoomPercent(double percent, that is not how percents work... a percent would be 110% not 1.1 this function and the above are now misleadingly named
Message was sent while issue was closed.
https://codereview.chromium.org/2422603003/diff/20001/chrome/common/extension... File chrome/common/extensions/api/settings_private.idl (right): https://codereview.chromium.org/2422603003/diff/20001/chrome/common/extension... chrome/common/extensions/api/settings_private.idl:82: static void setDefaultZoomPercent(double percent, On 2016/10/17 08:46:08, michaelpg wrote: > that is not how percents work... a percent would be 110% not 1.1 > > this function and the above are now misleadingly named +1
Message was sent while issue was closed.
followups done here: https://codereview.chromium.org/2430463002 https://codereview.chromium.org/2422603003/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/appearance_page/appearance_page.js (right): https://codereview.chromium.org/2422603003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/appearance_page/appearance_page.js:57: * @type {!DropdownMenuOptionList} On 2016/10/15 00:58:07, dpapad wrote: > Type annotation no longer accurate. Should be !Array<number> Done. https://codereview.chromium.org/2422603003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/appearance_page/appearance_page.js:65: 1 / 3, On 2016/10/17 08:46:08, michaelpg wrote: > when it comes to repeating decimals, is this safe? > > JSON pref store => double => JavaScript number => <option> string => JavaScript > number => double => JSON pref store safe enough, yes https://cs.chromium.org/chromium/src/content/common/page_zoom.cc?l=16 https://codereview.chromium.org/2422603003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/appearance_page/appearance_page.js:123: * @return {number} A zoom easier read by users. On 2016/10/17 08:46:08, michaelpg wrote: > {string} maybe it's not a string https://codereview.chromium.org/2422603003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/appearance_page/appearance_page.js:123: * @return {number} A zoom easier read by users. On 2016/10/15 00:58:07, dpapad wrote: > @private Done. https://codereview.chromium.org/2422603003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/appearance_page/appearance_page.js:126: return Math.round(zoom * 100); On 2016/10/17 08:46:08, michaelpg wrote: > return (zoom * 100).toFixed(); what's the functional difference? especially for the small set we're operating on?
Message was sent while issue was closed.
https://codereview.chromium.org/2422603003/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/appearance_page/appearance_page.js (right): https://codereview.chromium.org/2422603003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/appearance_page/appearance_page.js:65: 1 / 3, On 2016/10/17 19:20:26, Dan Beam wrote: > On 2016/10/17 08:46:08, michaelpg wrote: > > when it comes to repeating decimals, is this safe? > > > > JSON pref store => double => JavaScript number => <option> string => > JavaScript > > number => double => JSON pref store > > safe enough, yes > https://cs.chromium.org/chromium/src/content/common/page_zoom.cc?l=16 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 https://codereview.chromium.org/2422603003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/appearance_page/appearance_page.js:126: return Math.round(zoom * 100); On 2016/10/17 19:20:26, Dan Beam wrote: > On 2016/10/17 08:46:08, michaelpg wrote: > > return (zoom * 100).toFixed(); > > what's the functional difference? especially for the small set we're operating > on? toFixed returns a string. it's mathematically equivalent, but if we do return a number I think "format" is the wrong name.
Message was sent while issue was closed.
https://codereview.chromium.org/2422603003/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/appearance_page/appearance_page.js (right): https://codereview.chromium.org/2422603003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/appearance_page/appearance_page.js:65: 1 / 3, On 2016/10/17 19:58:15, michaelpg wrote: > On 2016/10/17 19:20:26, Dan Beam wrote: > > On 2016/10/17 08:46:08, michaelpg wrote: > > > when it comes to repeating decimals, is this safe? > > > > > > JSON pref store => double => JavaScript number => <option> string => > > JavaScript > > > number => double => JSON pref store > > > > safe enough, yes > > https://cs.chromium.org/chromium/src/content/common/page_zoom.cc?l=16 > > 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 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. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
