Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1241453005:
ChromeOS: Time zone setting dropdown should be grayed out once we enable the Automatically resolve … (Closed)
|
Created:
5 years, 5 months ago by Alexander Alekseev Modified:
5 years, 5 months ago CC:
chromium-reviews, dbeam+watch-options_chromium.org, michaelpg+watch-options_chromium.org, arv+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionChromeOS: Time zone setting dropdown should be grayed out once we enable the Automatically resolve timezone by geo location.
This CL fixes race in timezone section of settings page.
BUG=489588
TEST=manual
Committed: https://crrev.com/e5464ad117e2b1dce2ba6417884714a1fc90936c
Cr-Commit-Position: refs/heads/master@{#340043}
Patch Set 1 #
Total comments: 15
Patch Set 2 : Subscribe to Preferences JS interface. #Patch Set 3 : Remove duplicate setting. #Patch Set 4 : Convert updateTimezoneSectionState_() method to argumentless. #Messages
Total messages: 19 (7 generated)
alemate@chromium.org changed reviewers: + stevenjb@chromium.org
There is no way to catch event "$('resolve-timezone-by-geolocation').checked has
changed it's value" in JS, so dependent timezone dropdown sometimes gets out of
sync with checkbox.
This CL notifies JS of this event by a separate call.
Please review.
stevenjb@chromium.org changed reviewers: + michaelpg@chromium.org
stevenjb@chromium.org changed required reviewers: + michaelpg@chromium.org
-> michaelpg@ who is more familiar with this UI
$('resolve-timezone-by-geolocation') is tied to a preference, so you can listen
to that preference to trigger enabling/disabling the timezone dropdown.
https://codereview.chromium.org/1241453005/diff/1/chrome/browser/resources/op...
File chrome/browser/resources/options/browser_options.js (right):
https://codereview.chromium.org/1241453005/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/browser_options.js:113: * Current status of
"Resolve Timezone by Geolocation" checkbox.
Annotate: @private {boolean}
https://codereview.chromium.org/1241453005/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/browser_options.js:118: * True if system
timezone is managed by policy.
Annotate.
https://codereview.chromium.org/1241453005/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/browser_options.js:1665:
updateTimezoneSectionVisibility_: function() {
rename: this changes enabled state, not visibility
https://codereview.chromium.org/1241453005/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/browser_options.js:1667: if
(this.systemTimezoneIsManaged_) {
If this is only used here, can you make it a parameter instead of a member
variable?
https://codereview.chromium.org/1241453005/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/browser_options.js:1677:
this.resolveTimezoneByGeolocation_;
Same request.
https://codereview.chromium.org/1241453005/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/browser_options.js:1697: *
kResolveTimezoneByGeolocation preference is true or false.
nit: indent 4 spaces
https://codereview.chromium.org/1241453005/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/browser_options.js:1699:
onResolveTimezoneByGeolocationChanged_: function(value) {
This is a plain preference, so you can trigger this function in JavaScript
instead of involving the C++ handler.
Preferences.getInstance().addEventListener(
kResolveTimezoneByGeolocation,
this.onResolveTimezoneByGeolocationChanged_.bind(this));
See preferences.js for definition and this file for examples.
https://codereview.chromium.org/1241453005/diff/1/chrome/browser/resources/op... File chrome/browser/resources/options/browser_options.js (right): https://codereview.chromium.org/1241453005/diff/1/chrome/browser/resources/op... chrome/browser/resources/options/browser_options.js:113: * Current status of "Resolve Timezone by Geolocation" checkbox. On 2015/07/20 21:40:51, michaelpg wrote: > Annotate: @private {boolean} Done. https://codereview.chromium.org/1241453005/diff/1/chrome/browser/resources/op... chrome/browser/resources/options/browser_options.js:118: * True if system timezone is managed by policy. On 2015/07/20 21:40:51, michaelpg wrote: > Annotate. Done. https://codereview.chromium.org/1241453005/diff/1/chrome/browser/resources/op... chrome/browser/resources/options/browser_options.js:1665: updateTimezoneSectionVisibility_: function() { On 2015/07/20 21:40:51, michaelpg wrote: > rename: this changes enabled state, not visibility Done. https://codereview.chromium.org/1241453005/diff/1/chrome/browser/resources/op... chrome/browser/resources/options/browser_options.js:1667: if (this.systemTimezoneIsManaged_) { On 2015/07/20 21:40:51, michaelpg wrote: > If this is only used here, can you make it a parameter instead of a member > variable? Done. https://codereview.chromium.org/1241453005/diff/1/chrome/browser/resources/op... chrome/browser/resources/options/browser_options.js:1677: this.resolveTimezoneByGeolocation_; On 2015/07/20 21:40:51, michaelpg wrote: > Same request. Done. https://codereview.chromium.org/1241453005/diff/1/chrome/browser/resources/op... chrome/browser/resources/options/browser_options.js:1697: * kResolveTimezoneByGeolocation preference is true or false. On 2015/07/20 21:40:51, michaelpg wrote: > nit: indent 4 spaces Done. https://codereview.chromium.org/1241453005/diff/1/chrome/browser/resources/op... chrome/browser/resources/options/browser_options.js:1699: onResolveTimezoneByGeolocationChanged_: function(value) { On 2015/07/20 21:40:51, michaelpg wrote: > This is a plain preference, so you can trigger this function in JavaScript > instead of involving the C++ handler. > > Preferences.getInstance().addEventListener( > kResolveTimezoneByGeolocation, > this.onResolveTimezoneByGeolocationChanged_.bind(this)); > > See preferences.js for definition and this file for examples. This is what I was looking for! Thank you.
The CQ bit was checked by alemate@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1241453005/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/1241453005/diff/1/chrome/browser/resources/op... File chrome/browser/resources/options/browser_options.js (right): https://codereview.chromium.org/1241453005/diff/1/chrome/browser/resources/op... chrome/browser/resources/options/browser_options.js:1667: if (this.systemTimezoneIsManaged_) { On 2015/07/21 05:34:29, Alexander Alekseev wrote: > On 2015/07/20 21:40:51, michaelpg wrote: > > If this is only used here, can you make it a parameter instead of a member > > variable? > > Done. Sorry, I think my logic was wrong! There are 2 variables that can change separately, and this function needs to know both of them, so what you had made sense. So this function doesn't need parameters after all. You can remove them (like the code here) or leave them in (as they are in your latest patch), I don't care much one way or the other.
stevenjb@, PTAL I need your OWNER's approval.
OWNER lgtm
The CQ bit was checked by alemate@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from michaelpg@chromium.org Link to the patchset: https://codereview.chromium.org/1241453005/#ps60001 (title: "Convert updateTimezoneSectionState_() method to argumentless.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1241453005/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1241453005/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/e5464ad117e2b1dce2ba6417884714a1fc90936c Cr-Commit-Position: refs/heads/master@{#340043} |
