Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2488113002:
MD Settings: [further] robustify default zoom level handling (Closed)
|
Created:
4 years, 1 month ago by Dan Beam Modified:
4 years, 1 month ago Reviewers:
dpapad CC:
arv+watch_chromium.org, chromium-reviews, dbeam+watch-settings_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: [further] robustify default zoom level handling
R=dpapad@chromium.org
BUG=663661
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/e0e9385ba796fd29e1f0f11126a18e173364c864
Cr-Commit-Position: refs/heads/master@{#431149}
Patch Set 1 #
Total comments: 2
Patch Set 2 : . #
Total comments: 4
Messages
Total messages: 19 (11 generated)
Description was changed from ========== MD Settings: [further] robustify default zoom level handling R=dpapad@chromium.org BUG=663661 ========== to ========== MD Settings: [further] robustify default zoom level handling R=dpapad@chromium.org BUG=663661 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
do you want me to slap this behind a browser proxy to I can test?
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...
Putting chrome.settingsPrivate.getDefaultZoom behind a proxy (or even better removing the private API usage completely) sounds like a good idea. https://codereview.chromium.org/2488113002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/appearance_page/appearance_page.js (right): https://codereview.chromium.org/2488113002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/appearance_page/appearance_page.js:116: for (var i = 0; i < this.pageZoomLevels_.length; ++i) { If we have to iterate over zoom levels, would it make sense to add a binding instead? <option value="[[item]]" selected$="[[isSelected_(item,zoomLevel)]]">[[formatZoom_(item)]]%</option> properties: { ... zoomLevel: Number } isSelected_: function(item) { return Math.abs(item - this.zoomLevel) <= 0.001; }
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
so i'm kinda re-shuffling the appearance browser tests here: https://codereview.chromium.org/2489723006/ I'm going to make a CL based on both of these that adds a getDefaultZoom method to the browser proxy and harnesses it from the tests. you can review now or later, up to you. https://codereview.chromium.org/2488113002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/appearance_page/appearance_page.js (right): https://codereview.chromium.org/2488113002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/appearance_page/appearance_page.js:116: for (var i = 0; i < this.pageZoomLevels_.length; ++i) { On 2016/11/10 00:30:58, dpapad wrote: > If we have to iterate over zoom levels, would it make sense to add a binding > instead? > > <option value="[[item]]" > selected$="[[isSelected_(item,zoomLevel)]]">[[formatZoom_(item)]]%</option> > > properties: { > ... > zoomLevel: Number > } > > isSelected_: function(item) { > return Math.abs(item - this.zoomLevel) <= 0.001; > } Done.
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.
LGTM https://codereview.chromium.org/2488113002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/appearance_page/appearance_page.js (right): https://codereview.chromium.org/2488113002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/appearance_page/appearance_page.js:35: browserProxy_: Object, Nit: I recall that we are OK not putting browserProxy_ objects inside properties now. https://codereview.chromium.org/2488113002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/appearance_page/appearance_page.js:264: zoomValuesEqual_: function(zoom1, zoom2) { Nit (optional): I think only passing one param in this function, and using this.zoomLevel_ internally instead of 2nd param, makes it clearer that this binding triggers when/if this.zoomLevel_ changes (even without looking at the HTML).
https://codereview.chromium.org/2488113002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/appearance_page/appearance_page.js (right): https://codereview.chromium.org/2488113002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/appearance_page/appearance_page.js:35: browserProxy_: Object, On 2016/11/10 02:20:49, dpapad wrote: > Nit: I recall that we are OK not putting browserProxy_ objects inside properties > now. done in next patch https://codereview.chromium.org/2488113002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/appearance_page/appearance_page.js:264: zoomValuesEqual_: function(zoom1, zoom2) { On 2016/11/10 02:20:49, dpapad wrote: > Nit (optional): I think only passing one param in this function, and using > this.zoomLevel_ internally instead of 2nd param, makes it clearer that this > binding triggers when/if this.zoomLevel_ changes (even without looking at the > HTML). ehhhhhhhhhhhh, but then it's less like a straight-up port of content::ZoomValuesEqual(). honestly, most people need to look at the HTML to be sure either way (because that's where the computed binding that invokes this lives anyways)
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.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== MD Settings: [further] robustify default zoom level handling R=dpapad@chromium.org BUG=663661 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: [further] robustify default zoom level handling R=dpapad@chromium.org BUG=663661 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/e0e9385ba796fd29e1f0f11126a18e173364c864 Cr-Commit-Position: refs/heads/master@{#431149} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/e0e9385ba796fd29e1f0f11126a18e173364c864 Cr-Commit-Position: refs/heads/master@{#431149} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
