Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1984603002:
Allow ConfirmDialog to confirm disabling options. (Closed)
|
Created:
4 years, 7 months ago by dspaid Modified:
4 years, 7 months ago CC:
arv+watch_chromium.org, chromium-reviews, dbeam+watch-options_chromium.org, michaelpg+watch-options_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAllow ConfirmDialog to confirm disabling options.
BUG=612086
TEST=Manual testing. Preferences using this new functionality will
include their own tests.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/bf2c502f136f1f87a08d9a3546a893f6806b787a
Cr-Commit-Position: refs/heads/master@{#395780}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Addressed comments #
Total comments: 1
Patch Set 3 : Updated true/false/undefined logic #Messages
Total messages: 26 (12 generated)
Description was changed from ========== Allow ConfirmDialog to confirm disabling options. BUG=612086 TEST=Manual testing. Preferences using this new functionality will include their own tests. ========== to ========== Allow ConfirmDialog to confirm disabling options. BUG=612086 TEST=Manual testing. Preferences using this new functionality will include their own tests. CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
dspaid@chromium.org changed reviewers: + stevenjb@chromium.org
stevenjb@chromium.org changed reviewers: + dpapad@chromium.org
+dpapad@ who is working on the privacy page in the new Settings UI (which as near as I can tell is the only Options page that uses ConfirmDialog). FIWI, I would suggest other changes if we were not deprecating this UI soon anyway, but since we are, this seems fine. lgtm w/ nit https://codereview.chromium.org/1984603002/diff/1/chrome/browser/resources/op... File chrome/browser/resources/options/confirm_dialog.js (right): https://codereview.chromium.org/1984603002/diff/1/chrome/browser/resources/op... chrome/browser/resources/options/confirm_dialog.js:26: * trigger the confirmation dialog. Defaults to |true| if left |undefined|. indent
https://codereview.chromium.org/1984603002/diff/1/chrome/browser/resources/op... File chrome/browser/resources/options/confirm_dialog.js (right): https://codereview.chromium.org/1984603002/diff/1/chrome/browser/resources/op... chrome/browser/resources/options/confirm_dialog.js:25: * @param {boolean} opt_confirmValue The value to which changing should The naming of this param suggests that it is optional, which means that the annotation should be @param {boolean=} https://codereview.chromium.org/1984603002/diff/1/chrome/browser/resources/op... chrome/browser/resources/options/confirm_dialog.js:46: this.confirmValue_ = opt_confirmValue === undefined || This logic is fragile. In almost all JS APIs I have encountered, null/undefined is treated the same when it comes to optional parameters. Here it is not, if null is passed, this.confirmValue_ will be false, if undefined is passed, it will be true. One option would be to simply check for both null and undefined and treat them the same. https://codereview.chromium.org/1984603002/diff/1/chrome/browser/resources/op... chrome/browser/resources/options/confirm_dialog.js:107: this.confirmedPref_, this.confirmValue, true); Underscore missing! This is not referring to the member var this.confirmValue_. I am hoping that the JS Compiler would catch this, or a test.
https://codereview.chromium.org/1984603002/diff/1/chrome/browser/resources/op... File chrome/browser/resources/options/confirm_dialog.js (right): https://codereview.chromium.org/1984603002/diff/1/chrome/browser/resources/op... chrome/browser/resources/options/confirm_dialog.js:25: * @param {boolean} opt_confirmValue The value to which changing should On 2016/05/16 17:09:53, dpapad wrote: > The naming of this param suggests that it is optional, which means that the > annotation should be > @param {boolean=} Done. https://codereview.chromium.org/1984603002/diff/1/chrome/browser/resources/op... chrome/browser/resources/options/confirm_dialog.js:26: * trigger the confirmation dialog. Defaults to |true| if left |undefined|. On 2016/05/16 16:19:48, stevenjb wrote: > indent Done. https://codereview.chromium.org/1984603002/diff/1/chrome/browser/resources/op... chrome/browser/resources/options/confirm_dialog.js:46: this.confirmValue_ = opt_confirmValue === undefined || On 2016/05/16 17:09:53, dpapad wrote: > This logic is fragile. In almost all JS APIs I have encountered, null/undefined > is treated the same when it comes to optional parameters. Here it is not, if > null is passed, this.confirmValue_ will be false, if undefined is passed, it > will be true. > > One option would be to simply check for both null and undefined and treat them > the same. Good point. changing === to == should also do the trick. https://codereview.chromium.org/1984603002/diff/1/chrome/browser/resources/op... chrome/browser/resources/options/confirm_dialog.js:107: this.confirmedPref_, this.confirmValue, true); On 2016/05/16 17:09:53, dpapad wrote: > Underscore missing! This is not referring to the member var this.confirmValue_. > I am hoping that the JS Compiler would catch this, or a test. Done. Oddly enough the compiler didn't seem to complain, though maybe I was just looking in the wrong place.
LGTM, with suggestion. https://codereview.chromium.org/1984603002/diff/20001/chrome/browser/resource... File chrome/browser/resources/options/confirm_dialog.js (right): https://codereview.chromium.org/1984603002/diff/20001/chrome/browser/resource... chrome/browser/resources/options/confirm_dialog.js:47: this.confirmValue_ = opt_confirmValue == undefined || "null == undefined" VS "null === undefined" behavior belongs in JS "bad parts" IMO, since it is not intuitive at all. Other libraries address this by hiding the complexity behind a utility isDef() method (like goog.isDef). My suggestion is to change it to the more intuitive and equivalent this.confirmValue_ = opt_confirmValue === false ? false : true;
The CQ bit was checked by dspaid@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org, dpapad@chromium.org Link to the patchset: https://codereview.chromium.org/1984603002/#ps40001 (title: "Updated true/false/undefined logic")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1984603002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1984603002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by dspaid@chromium.org to run a CQ dry run
The CQ bit was unchecked by dspaid@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1984603002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1984603002/40001
The CQ bit was checked by dspaid@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1984603002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1984603002/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 dspaid@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1984603002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1984603002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Allow ConfirmDialog to confirm disabling options. BUG=612086 TEST=Manual testing. Preferences using this new functionality will include their own tests. CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== Allow ConfirmDialog to confirm disabling options. BUG=612086 TEST=Manual testing. Preferences using this new functionality will include their own tests. CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/bf2c502f136f1f87a08d9a3546a893f6806b787a Cr-Commit-Position: refs/heads/master@{#395780} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/bf2c502f136f1f87a08d9a3546a893f6806b787a Cr-Commit-Position: refs/heads/master@{#395780} |
