Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1486953003:
Support numbers in settings-checkbox (Closed)
|
Created:
5 years ago by stevenjb Modified:
5 years ago CC:
chromium-reviews, michaelpg+watch-md-settings_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_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. |
DescriptionSupport numbers in settings-checkbox
BUG=521791
Committed: https://crrev.com/718499570a952cf2e95a4d6b695bd931d088ab05
Cr-Commit-Position: refs/heads/master@{#362836}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Fix type + add test #Patch Set 3 : Ensure type of boolean pref also #
Total comments: 8
Patch Set 4 : Rebase + Feedback #
Messages
Total messages: 30 (13 generated)
stevenjb@chromium.org changed reviewers: + dbeam@chromium.org, michaelpg@chromium.org
lgtm (would be cool to test this, though...) https://codereview.chromium.org/1486953003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/controls/settings_checkbox.js (right): https://codereview.chromium.org/1486953003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/controls/settings_checkbox.js:94: // a bolean or a number. boolean
Excellent point. Test added, PTAL. https://codereview.chromium.org/1486953003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/controls/settings_checkbox.js (right): https://codereview.chromium.org/1486953003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/controls/settings_checkbox.js:94: // a bolean or a number. On 2015/12/02 05:11:59, Dan Beam wrote: > boolean Done.
lgtm https://codereview.chromium.org/1486953003/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/controls/settings_checkbox.js (right): https://codereview.chromium.org/1486953003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/controls/settings_checkbox.js:92: /** @type {boolean} */ var newValue = this.getNewValue_(this.checked); looks like @type is redundant with @return of getNewValue_
https://codereview.chromium.org/1486953003/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/controls/settings_checkbox.js (right): https://codereview.chromium.org/1486953003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/controls/settings_checkbox.js:92: /** @type {boolean} */ var newValue = this.getNewValue_(this.checked); On 2015/12/02 18:54:50, michaelpg wrote: > looks like @type is redundant with @return of getNewValue_ It is. I added it strictly as a comment to remind us that newValue is always a boolean (whereas pref.value might be a boolean or number).
The CQ bit was checked by stevenjb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/1486953003/#ps40001 (title: "Ensure type of boolean pref also")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1486953003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1486953003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by stevenjb@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1486953003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1486953003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Support numbers in settings-checknox BUG=521791 ========== to ========== Support numbers in settings-checkbox BUG=521791 ==========
https://codereview.chromium.org/1486953003/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/settings/checkbox_tests.js (right): https://codereview.chromium.org/1486953003/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/checkbox_tests.js:48: assertEquals(true, pref.value); why is this better? https://codereview.chromium.org/1486953003/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/checkbox_tests.js:78: testElementNum.set('pref', prefNum); why can't we just re-use the testElement fixture and call testElement.set('pref', prefNum); ? https://codereview.chromium.org/1486953003/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/checkbox_tests.js:79: document.body.appendChild(testElementNum); why does this need to be inserted into the DOM?
https://codereview.chromium.org/1486953003/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/settings/checkbox_tests.js (right): https://codereview.chromium.org/1486953003/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/checkbox_tests.js:48: assertEquals(true, pref.value); On 2015/12/02 21:48:57, Dan Beam wrote: > why is this better? It's not. I made the incorrect assumption that assertFalse(0) would pass. https://codereview.chromium.org/1486953003/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/checkbox_tests.js:78: testElementNum.set('pref', prefNum); On 2015/12/02 21:48:57, Dan Beam wrote: > why can't we just re-use the testElement fixture and call > > testElement.set('pref', prefNum); > > ? We can. Done. https://codereview.chromium.org/1486953003/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/checkbox_tests.js:79: document.body.appendChild(testElementNum); On 2015/12/02 21:48:57, Dan Beam wrote: > why does this need to be inserted into the DOM? Acknowledged.
The CQ bit was checked by stevenjb@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/1486953003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1486953003/60001
lgtm++
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by stevenjb@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/1486953003/#ps60001 (title: "Rebase + Feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1486953003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1486953003/60001
Message was sent while issue was closed.
Description was changed from ========== Support numbers in settings-checkbox BUG=521791 ========== to ========== Support numbers in settings-checkbox BUG=521791 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Support numbers in settings-checkbox BUG=521791 ========== to ========== Support numbers in settings-checkbox BUG=521791 Committed: https://crrev.com/718499570a952cf2e95a4d6b695bd931d088ab05 Cr-Commit-Position: refs/heads/master@{#362836} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/718499570a952cf2e95a4d6b695bd931d088ab05 Cr-Commit-Position: refs/heads/master@{#362836} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
