Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2277993002:
Small fixes for the lock screen md-settings page. (Closed)
|
Created:
4 years, 4 months ago by jdufault Modified:
4 years, 3 months ago Reviewers:
tommycli CC:
chromium-reviews, vabr+watchlistpasswordmanager_chromium.org, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, arv+watch_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org, gcasto+watchlist_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSmall fixes for the lock screen md-settings page.
BUG=603217
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/09c7ebf5c8d47e2f1335f593e3abb47e013cd58d
Cr-Commit-Position: refs/heads/master@{#414608}
Patch Set 1 : Initial upload #Patch Set 2 : Adjust strings as well #
Total comments: 4
Patch Set 3 : Address comments #
Total comments: 6
Patch Set 4 : Address comments #
Messages
Total messages: 33 (23 generated)
Description was changed from ========== Small fixes for the lock screen password prompt dialog BUG= ========== to ========== Small fixes for the lock screen password prompt dialog BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by jdufault@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.
The CQ bit was checked by jdufault@chromium.org to run a CQ dry run
Patchset #1 (id:1) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Small fixes for the lock screen password prompt dialog BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Small fixes for the lock screen password prompt dialog BUG=603217 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
jdufault@chromium.org changed reviewers: + tommycli@chromium.org
Description was changed from ========== Small fixes for the lock screen password prompt dialog BUG=603217 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Small fixes for the lock screen md-settings page. BUG=603217 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
tommycli@ PTAL. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by jdufault@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...
https://codereview.chromium.org/2277993002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/password_prompt_dialog.js (left): https://codereview.chromium.org/2277993002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/people_page/password_prompt_dialog.js:111: this.setModes_ = undefined; We don't care about clearing setModes_ anymore in this use case you mentioned in the comment? https://codereview.chromium.org/2277993002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/password_prompt_dialog.js (right): https://codereview.chromium.org/2277993002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/people_page/password_prompt_dialog.js:100: close: function() { This is never called from outside this class right? In which case make it private and onCancelTap_?
The CQ bit was checked by jdufault@chromium.org to run a CQ dry run
https://codereview.chromium.org/2277993002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/password_prompt_dialog.js (left): https://codereview.chromium.org/2277993002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/people_page/password_prompt_dialog.js:111: this.setModes_ = undefined; On 2016/08/25 23:13:32, tommycli wrote: > We don't care about clearing setModes_ anymore in this use case you mentioned in > the comment? Right, the on-change event binding has been removed. https://codereview.chromium.org/2277993002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/password_prompt_dialog.js (right): https://codereview.chromium.org/2277993002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/people_page/password_prompt_dialog.js:100: close: function() { On 2016/08/25 23:13:32, tommycli wrote: > This is never called from outside this class right? In which case make it > private and onCancelTap_? Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2277993002/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/password_prompt_dialog.js (right): https://codereview.chromium.org/2277993002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/people_page/password_prompt_dialog.js:146: password = ''; err... does this actually do anything? I assume you want to clear the this.password_ variable immediately and not in this timeout right? https://codereview.chromium.org/2277993002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/people_page/password_prompt_dialog.js:152: this.$.dialog.close(); You no longer wish to clear the password_ member variable here?
https://codereview.chromium.org/2277993002/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/password_prompt_dialog.js (right): https://codereview.chromium.org/2277993002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/people_page/password_prompt_dialog.js:117: checkPassword_: function() { One more thing: It would be good to change this to submitPassword_ since that's the new meaning right? and updated comment too about it closing the dialog
The CQ bit was checked by jdufault@chromium.org to run a CQ dry run
https://codereview.chromium.org/2277993002/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/password_prompt_dialog.js (right): https://codereview.chromium.org/2277993002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/people_page/password_prompt_dialog.js:117: checkPassword_: function() { On 2016/08/25 23:35:21, tommycli wrote: > One more thing: It would be good to change this to submitPassword_ since that's > the new meaning right? and updated comment too about it closing the dialog Done. https://codereview.chromium.org/2277993002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/people_page/password_prompt_dialog.js:146: password = ''; On 2016/08/25 23:34:25, tommycli wrote: > err... does this actually do anything? I assume you want to clear the > this.password_ variable immediately and not in this timeout right? This makes it so that calling the closure from a stored variable will fail. this.password_ has been cleared a long time ago. https://codereview.chromium.org/2277993002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/people_page/password_prompt_dialog.js:152: this.$.dialog.close(); On 2016/08/25 23:34:25, tommycli wrote: > You no longer wish to clear the password_ member variable here? Added clarifying comment.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by jdufault@chromium.org
The CQ bit was checked by jdufault@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.
Description was changed from ========== Small fixes for the lock screen md-settings page. BUG=603217 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Small fixes for the lock screen md-settings page. BUG=603217 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Small fixes for the lock screen md-settings page. BUG=603217 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Small fixes for the lock screen md-settings page. BUG=603217 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/09c7ebf5c8d47e2f1335f593e3abb47e013cd58d Cr-Commit-Position: refs/heads/master@{#414608} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/09c7ebf5c8d47e2f1335f593e3abb47e013cd58d Cr-Commit-Position: refs/heads/master@{#414608} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
