|
|
Created:
5 years, 6 months ago by Oren Blasberg Modified:
5 years, 6 months ago CC:
chromium-reviews, khorimoto+watch-md-settings_chromium.org, michaelpg+watch-md-settings_chromium.org, jhawkins+watch-md-settings_chromium.org, orenb+watch-md-settings_chromium.org, arv+watch_chromium.org, stevenjb+watch-md-settings_chromium.org, jlklein+watch-md-settings_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptioncr-settings-checkbox: Add 'inverted' property.
This will allow us to use it in the Users page (and potentially others) for the
prefs where the checkbox being checked means the boolean pref is disabled, and
unchecked means enabled.
Example usage:
<cr-settings-checkbox
pref="{{prefs.cros.accounts.allowGuest}}" inverted>
</cr-settings-checkbox>
BUG=495858
Committed: https://crrev.com/d9f0ed3087ef9d25bcaed6ed749979fdec9fdf20
Cr-Commit-Position: refs/heads/master@{#333110}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Address Jeremy's comments #Patch Set 3 : Keep pref's default unset, but fix the error. #Patch Set 4 : Rename that method. #
Messages
Total messages: 20 (5 generated)
orenb@chromium.org changed reviewers: + jlklein@chromium.org
Make sure you test this out in your other CL. https://codereview.chromium.org/1156783004/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/checkbox/checkbox.js (right): https://codereview.chromium.org/1156783004/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/checkbox/checkbox.js:27: value: function() { return {}; } Why do we need this? Doesn't it break the pref-tracker? https://codereview.chromium.org/1156783004/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/checkbox/checkbox.js:61: prefValueChanged: function(prefValue) { This and the other 2 new functions can be private. https://codereview.chromium.org/1156783004/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/checkbox/checkbox.js:71: computeValue: function(val) { For some reason the name computeValue confuses me because it's used for both checked and pref.value. Maybe "invertIfNeeded_"?
jhawkins@chromium.org changed reviewers: + jhawkins@chromium.org
Can you add a usage in this CL? It would be good to see how it's supposed to be used.
On 2015/06/04 01:14:41, James Hawkins wrote: > Can you add a usage in this CL? It would be good to see how it's supposed to be > used. Or I guess you can also just add an example to the CL description.
On 2015/06/04 00:49:54, Jeremy Klein wrote: > Make sure you test this out in your other CL. I've been testing this with the a11y_page. I confirmed that it works correctly with several checkboxes there. On 2015/06/04 01:15:05, James Hawkins wrote: > On 2015/06/04 01:14:41, James Hawkins wrote: > > Can you add a usage in this CL? It would be good to see how it's supposed to > be > > used. > > Or I guess you can also just add an example to the CL description. Done.x
https://codereview.chromium.org/1156783004/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/checkbox/checkbox.js (right): https://codereview.chromium.org/1156783004/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/checkbox/checkbox.js:27: value: function() { return {}; } On 2015/06/04 00:49:54, Jeremy Klein wrote: > Why do we need this? Doesn't it break the pref-tracker? Oddly, without the initial value, I get an error in checkedChanged_ that 'this.pref' is undefined. I'm not sure I understand why; maybe we can look at it tomorrow. Having this initial value doesn't seem to break the pref tracker; could you touch upon why you think it would? https://codereview.chromium.org/1156783004/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/checkbox/checkbox.js:61: prefValueChanged: function(prefValue) { On 2015/06/04 00:49:54, Jeremy Klein wrote: > This and the other 2 new functions can be private. Done. https://codereview.chromium.org/1156783004/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/checkbox/checkbox.js:71: computeValue: function(val) { On 2015/06/04 00:49:54, Jeremy Klein wrote: > For some reason the name computeValue confuses me because it's used for both > checked and pref.value. Maybe "invertIfNeeded_"? Ah ok. It's not actually inverting though; it's just returning a value, so how about "getPrefValue_" or "getValueToPersist_"?
https://codereview.chromium.org/1156783004/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/checkbox/checkbox.js (right): https://codereview.chromium.org/1156783004/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/checkbox/checkbox.js:27: value: function() { return {}; } On 2015/06/04 16:10:04, Oren Blasberg wrote: > On 2015/06/04 00:49:54, Jeremy Klein wrote: > > Why do we need this? Doesn't it break the pref-tracker? > > Oddly, without the initial value, I get an error in checkedChanged_ that > 'this.pref' is undefined. I'm not sure I understand why; maybe we can look at it > tomorrow. > > Having this initial value doesn't seem to break the pref tracker; could you > touch upon why you think it would? Sorry, by "breaking" I mean preventing real errors from being found. If you set a default here to {}, this line will always be false: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... and so the errors won't get picked up. What's the benefit of the default value? https://codereview.chromium.org/1156783004/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/checkbox/checkbox.js:71: computeValue: function(val) { On 2015/06/04 16:10:04, Oren Blasberg wrote: > On 2015/06/04 00:49:54, Jeremy Klein wrote: > > For some reason the name computeValue confuses me because it's used for both > > checked and pref.value. Maybe "invertIfNeeded_"? > > Ah ok. It's not actually inverting though; it's just returning a value, so how > about "getPrefValue_" or "getValueToPersist_"? The this is that it doesn't only get the prefValue or the value to persist because you're using it to go both directions: pref -> this.checked and this.checked -> pref
https://codereview.chromium.org/1156783004/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/checkbox/checkbox.js (right): https://codereview.chromium.org/1156783004/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/checkbox/checkbox.js:27: value: function() { return {}; } On 2015/06/04 17:20:16, Jeremy Klein wrote: > On 2015/06/04 16:10:04, Oren Blasberg wrote: > > On 2015/06/04 00:49:54, Jeremy Klein wrote: > > > Why do we need this? Doesn't it break the pref-tracker? > > > > Oddly, without the initial value, I get an error in checkedChanged_ that > > 'this.pref' is undefined. I'm not sure I understand why; maybe we can look at > it > > tomorrow. > > > > Having this initial value doesn't seem to break the pref tracker; could you > > touch upon why you think it would? > > Sorry, by "breaking" I mean preventing real errors from being found. If you set > a default here to {}, this line will always be false: > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... > > and so the errors won't get picked up. What's the benefit of the default value? Oh, I see. Yes, the validation would always succeed. There isn't a 'benefit' to having {} here, except that it prevents the error I mentioned in the previous comment, in checkedChanged_. But I still don't get why that error happens in the first place, since checkedChanged_ should only be getting called if the 'checked' actually changes (which I don't think happens on initialization?)... Let's look at this in person later. https://codereview.chromium.org/1156783004/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/checkbox/checkbox.js:71: computeValue: function(val) { On 2015/06/04 17:20:16, Jeremy Klein wrote: > On 2015/06/04 16:10:04, Oren Blasberg wrote: > > On 2015/06/04 00:49:54, Jeremy Klein wrote: > > > For some reason the name computeValue confuses me because it's used for both > > > checked and pref.value. Maybe "invertIfNeeded_"? > > > > Ah ok. It's not actually inverting though; it's just returning a value, so how > > about "getPrefValue_" or "getValueToPersist_"? > > The this is that it doesn't only get the prefValue or the value to persist > because you're using it to go both directions: > > pref -> this.checked > and this.checked -> pref Ok, how about "getUpdatedValue_"? "getValueBasedOnInversion_"? ... Because this doesn't actually do inversion, it just returns a bool I am hesitant to call it "invertIfNeeded_". As you pointed out, the field that gets inverted (if needed) depends on who called the method.
Ok, resolved the checkboxChanged_ stuff offline and renamed the method.
lgtm
The CQ bit was checked by orenb@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1156783004/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
The CQ bit was checked by orenb@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1156783004/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/d9f0ed3087ef9d25bcaed6ed749979fdec9fdf20 Cr-Commit-Position: refs/heads/master@{#333110} |