MD Settings: Runtime error when setting zoom to 110% |
||||
Issue descriptionRepro steps: 1) Navigate to chrome://md-settings 2) Set zoom to 110% 3) Reload the page. Observe error in the console (see attachment). Investigated a bit and some odd things happen. 1) Every time chrome://md-settings is reloaded a call to chrome.settingsPrivate.setDefaultZoomPercent [1] is made. This seems odd because the user did not actually change the zoom. Is this expected? 2) When the previous zoom value (before reloading) is set to 110%, setDefaultZoomPercent() is invoked with 110.00000000000001 (after reloading) which is triggering the error. I have not figured out where does this number come from yet. [1] https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/appearance_page/appearance_page.js?l=216
,
Oct 3 2016
The non-integer value is coming from chrome.settingsPrivate.getDefaultZoomPercent(), so perhaps a bug in the C++? @dschuyler: I took a look at the original CL at [1]. Per my understanding we are explicitly setting the defalut zoom level on startup at [2]. It seems that we should only be setting it when the user changes it, but I don't have the full context (CL description does not mention anything about Zoom functionality either). In any case, the exception is thrown from [3] because of the non integer zoom level value coming from getDefaultZoomLevel(). [1] https://codereview.chromium.org/1420533011 [2] https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/appearance_page/appearance_page.js?l=126. [3] https://cs.chromium.org/chromium/src/extensions/renderer/resources/schema_utils.js?l=112-115
,
Oct 6 2016
Issue 629477 has been merged into this issue.
,
Oct 10 2016
The non-integer value seems to be a rounding error when transmitting a double C++ var to JS. Specifically at [1] the 'zoom' variable is 110, but when transmitted to JS, it comes out as 110.0000001. Perhaps a bug in some deeper layer (at [2] maybe?). @dschuyler: A simple fix is shown at [3], where std::round() is called before passing the value to JS. Do you think this fix is reasonable? [1] https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/settings_private/settings_private_delegate.cc?l=62 [2] https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/settings_private/settings_private_api.cc?l=124 [3] https://codereview.chromium.org/2409753003
,
Oct 10 2016
With a float value 110.0000001 is essentially 110 for our purposes. I'd go further than saying that rounding the value is reasonable, I propose that it is correct. Thanks!
,
Oct 10 2016
,
Oct 15 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7577ff2509a7a5d91ec319f12a42dd1acdb1a41f commit 7577ff2509a7a5d91ec319f12a42dd1acdb1a41f Author: dpapad <dpapad@chromium.org> Date: Sat Oct 15 00:41:22 2016 MD Settings: Stop calling chrome.settingsPrivate.setDefaultZoomLevel on startup. BUG= 652483 , 654588 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2403353002 Cr-Commit-Position: refs/heads/master@{#425519} [modify] https://crrev.com/7577ff2509a7a5d91ec319f12a42dd1acdb1a41f/chrome/browser/resources/settings/appearance_page/appearance_page.html [modify] https://crrev.com/7577ff2509a7a5d91ec319f12a42dd1acdb1a41f/chrome/browser/resources/settings/appearance_page/appearance_page.js
,
Oct 15 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6a3ab25a3152c4ab095c78347c70fe082f6a1743 commit 6a3ab25a3152c4ab095c78347c70fe082f6a1743 Author: dpapad <dpapad@chromium.org> Date: Sat Oct 15 06:48:24 2016 MD Settings: chrome.settingsPrivate, fix rounding error. BUG= 652483 Review-Url: https://codereview.chromium.org/2409753003 Cr-Commit-Position: refs/heads/master@{#425552} [modify] https://crrev.com/6a3ab25a3152c4ab095c78347c70fe082f6a1743/chrome/browser/extensions/api/settings_private/settings_private_delegate.cc
,
Oct 15 2016
|
||||
►
Sign in to add a comment |
||||
Comment 1 by dpa...@chromium.org
, Oct 3 2016