|
|
Created:
3 years, 11 months ago by hcarmona Modified:
3 years, 10 months ago CC:
dpapad, arv+watch_chromium.org, chromium-reviews, dbeam+watch-settings_chromium.org, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, stevenjb+watch-md-settings_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionLoad Passwords and Autofill in the corresponding sub page.
Moving the load into the sub page fixes the delay that is seen when
navigating into the passwords sub page. This has the added advantage
that items are not loaded at the top page.
BUG=665748
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2627123002
Cr-Commit-Position: refs/heads/master@{#451096}
Committed: https://chromium.googlesource.com/chromium/src/+/7320f5e364e6c02b2d044612e4e488d70bb1e24f
Patch Set 1 : Fix Tests #Patch Set 2 : Undo indent for better diff #
Total comments: 14
Patch Set 3 : feedback #
Total comments: 13
Patch Set 4 : fix missing ] #Patch Set 5 : feedback + rebase #
Total comments: 12
Patch Set 6 : fix nits #Patch Set 7 : rebase #
Total comments: 9
Patch Set 8 : feedback #
Total comments: 12
Patch Set 9 : Feedback #
Total comments: 9
Patch Set 10 : Add missing @private #
Total comments: 6
Patch Set 11 : feedbavk #Patch Set 12 : rebase #Messages
Total messages: 102 (72 generated)
Description was changed from ========== Load Passwords and Autofill in the corresponding sub page. Moving the load into the sub page fixes the delay that is seen when navigating into the passwords sub page. This has the added advantage that items are not loaded at the top page. BUG=665748 ========== to ========== Load Passwords and Autofill in the corresponding sub page. Moving the load into the sub page fixes the delay that is seen when navigating into the passwords sub page. This has the added advantage that items are not loaded at the top page. BUG=665748 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by hcarmona@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: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by hcarmona@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: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by hcarmona@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...
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
The CQ bit was checked by hcarmona@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...
hcarmona@chromium.org changed reviewers: + dbeam@chromium.org
Sorry for big patch! PTAL. Most changes are moving code around. Had to update tests and code no longer fires events (simpler, yay!)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
looks pretty good! https://codereview.chromium.org/2627123002/diff/60001/chrome/test/data/webui/... File chrome/test/data/webui/settings/passwords_and_autofill_fake_data.js (right): https://codereview.chromium.org/2627123002/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/settings/passwords_and_autofill_fake_data.js:132: function PasswordManagerExpectations() {}; don't put a ; after function Delcarations() {} https://codereview.chromium.org/2627123002/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/settings/passwords_and_autofill_fake_data.js:158: }; no ; https://codereview.chromium.org/2627123002/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/settings/passwords_and_autofill_fake_data.js:243: function AutofillManagerExpectations() {}; ^ https://codereview.chromium.org/2627123002/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/settings/passwords_and_autofill_fake_data.js:263: }; ^ https://codereview.chromium.org/2627123002/diff/60001/chrome/test/data/webui/... File chrome/test/data/webui/settings/passwords_and_forms_browsertest.js (right): https://codereview.chromium.org/2627123002/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/settings/passwords_and_forms_browsertest.js:66: element.$$('template[route-path="/passwords"').if = true; did you run this? there should be a ] at the end element.$$('template[route-path="/passwords"]') https://codereview.chromium.org/2627123002/diff/60001/chrome/test/data/webui/... File chrome/test/data/webui/settings/settings_passwords_section_browsertest.js (right): https://codereview.chromium.org/2627123002/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_passwords_section_browsertest.js:254: self.passwordManager.removeSavedPassword = function(detail) { wait, what is this doing? redefining a part of a class at runtime?
The CQ bit was checked by hcarmona@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/2627123002/diff/60001/chrome/test/data/webui/... File chrome/test/data/webui/settings/passwords_and_autofill_fake_data.js (right): https://codereview.chromium.org/2627123002/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/settings/passwords_and_autofill_fake_data.js:132: function PasswordManagerExpectations() {}; On 2017/01/26 19:10:44, Dan Beam wrote: > don't put a ; after function Delcarations() {} Done. https://codereview.chromium.org/2627123002/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/settings/passwords_and_autofill_fake_data.js:158: }; On 2017/01/26 19:10:44, Dan Beam wrote: > no ; Done. https://codereview.chromium.org/2627123002/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/settings/passwords_and_autofill_fake_data.js:243: function AutofillManagerExpectations() {}; On 2017/01/26 19:10:44, Dan Beam wrote: > ^ Done. https://codereview.chromium.org/2627123002/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/settings/passwords_and_autofill_fake_data.js:263: }; On 2017/01/26 19:10:44, Dan Beam wrote: > ^ Done. https://codereview.chromium.org/2627123002/diff/60001/chrome/test/data/webui/... File chrome/test/data/webui/settings/passwords_and_forms_browsertest.js (right): https://codereview.chromium.org/2627123002/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/settings/passwords_and_forms_browsertest.js:66: element.$$('template[route-path="/passwords"').if = true; On 2017/01/26 19:10:44, Dan Beam wrote: > did you run this? there should be a ] at the end > > element.$$('template[route-path="/passwords"]') Fixed... but it finds the right element, you can also ommit the last quote... That's weird behavior, so I've added an assert too. https://codereview.chromium.org/2627123002/diff/60001/chrome/test/data/webui/... File chrome/test/data/webui/settings/settings_passwords_section_browsertest.js (right): https://codereview.chromium.org/2627123002/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_passwords_section_browsertest.js:254: self.passwordManager.removeSavedPassword = function(detail) { On 2017/01/26 19:10:45, Dan Beam wrote: > wait, what is this doing? redefining a part of a class at runtime? Yes. That's weird. Updated. https://codereview.chromium.org/2627123002/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/settings/settings_passwords_section_browsertest.js (right): https://codereview.chromium.org/2627123002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_passwords_section_browsertest.js:47: PasswordManagerImpl.instance_ = null; Moved this here to clean up after the test.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2627123002/diff/60001/chrome/test/data/webui/... File chrome/test/data/webui/settings/passwords_and_forms_browsertest.js (right): https://codereview.chromium.org/2627123002/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/settings/passwords_and_forms_browsertest.js:66: element.$$('template[route-path="/passwords"').if = true; On 2017/01/26 21:34:47, hcarmona wrote: > On 2017/01/26 19:10:44, Dan Beam wrote: > > did you run this? there should be a ] at the end > > > > element.$$('template[route-path="/passwords"]') > > Fixed... but it finds the right element, you can also ommit the last quote... > That's weird behavior, so I've added an assert too. well, there's no difference, because returning null would blow up if you try to access .if of null https://codereview.chromium.org/2627123002/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/settings/passwords_and_forms_browsertest.js (right): https://codereview.chromium.org/2627123002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/settings/passwords_and_forms_browsertest.js:67: var passwordsElement = element.$$('template[route-path="/passwords"'); this still isn't correct element.$$('template[route-path="/passwords"]'); ^ https://codereview.chromium.org/2627123002/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/settings/settings_passwords_section_browsertest.js (left): https://codereview.chromium.org/2627123002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_passwords_section_browsertest.js:402: passwordsSection.addEventListener('remove-password-exception', why can't we just use events like we were before? https://codereview.chromium.org/2627123002/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/settings/settings_passwords_section_browsertest.js (right): https://codereview.chromium.org/2627123002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_passwords_section_browsertest.js:47: PasswordManagerImpl.instance_ = null; On 2017/01/26 21:34:47, hcarmona wrote: > Moved this here to clean up after the test. we're just re-creating on setup, why does this matter?
The CQ bit was checked by hcarmona@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/2627123002/diff/60001/chrome/test/data/webui/... File chrome/test/data/webui/settings/passwords_and_forms_browsertest.js (right): https://codereview.chromium.org/2627123002/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/settings/passwords_and_forms_browsertest.js:66: element.$$('template[route-path="/passwords"').if = true; On 2017/01/27 01:11:32, Dan Beam wrote: > On 2017/01/26 21:34:47, hcarmona wrote: > > On 2017/01/26 19:10:44, Dan Beam wrote: > > > did you run this? there should be a ] at the end > > > > > > element.$$('template[route-path="/passwords"]') > > > > Fixed... but it finds the right element, you can also ommit the last quote... > > That's weird behavior, so I've added an assert too. > > well, there's no difference, because returning null would blow up if you try to > access .if of null Removed asserts to keep cleaner. https://codereview.chromium.org/2627123002/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/settings/passwords_and_forms_browsertest.js (right): https://codereview.chromium.org/2627123002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/settings/passwords_and_forms_browsertest.js:67: var passwordsElement = element.$$('template[route-path="/passwords"'); On 2017/01/27 01:11:32, Dan Beam wrote: > this still isn't correct > > element.$$('template[route-path="/passwords"]'); > ^ Sorry about that. Done. https://codereview.chromium.org/2627123002/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/settings/settings_passwords_section_browsertest.js (left): https://codereview.chromium.org/2627123002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_passwords_section_browsertest.js:402: passwordsSection.addEventListener('remove-password-exception', On 2017/01/27 01:11:32, Dan Beam wrote: > why can't we just use events like we were before? The event was fired so the parent element could handle it because the passwordsSection did not know about the passwordManager. The passwordsSection can interact directly with the passwordManager now that it has a reference to it. I can put the event back in, but it seems unnecessary now. https://codereview.chromium.org/2627123002/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/settings/settings_passwords_section_browsertest.js (right): https://codereview.chromium.org/2627123002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_passwords_section_browsertest.js:47: PasswordManagerImpl.instance_ = null; On 2017/01/27 01:11:32, Dan Beam wrote: > On 2017/01/26 21:34:47, hcarmona wrote: > > Moved this here to clean up after the test. > > we're just re-creating on setup, why does this matter? PasswordManagerImpl.instance_ is global. If 2 tests rely on this and it's not reset, then we can get into a state where one test will depend on the other. This leads to weird failures that depend on test order.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
still thinking about your onRemoveSavedPassword stuff... https://codereview.chromium.org/2627123002/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/settings/settings_passwords_section_browsertest.js (right): https://codereview.chromium.org/2627123002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_passwords_section_browsertest.js:47: PasswordManagerImpl.instance_ = null; On 2017/01/27 18:28:16, hcarmona wrote: > On 2017/01/27 01:11:32, Dan Beam wrote: > > On 2017/01/26 21:34:47, hcarmona wrote: > > > Moved this here to clean up after the test. > > > > we're just re-creating on setup, why does this matter? > > PasswordManagerImpl.instance_ is global. If 2 tests rely on this and it's not > reset, then we can get into a state where one test will depend on the other. > This leads to weird failures that depend on test order. i mean, you can leave this in, but nobody else does it ;) overriding a global instance: https://cs.chromium.org/search/?q=%22instance_+%3D+%22+chrome/test/data/webui... tearing down a global instance: https://cs.chromium.org/search/?q=%22instance_+%3D+null%22+chrome/test/data/w...
https://codereview.chromium.org/2627123002/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/settings/settings_passwords_section_browsertest.js (right): https://codereview.chromium.org/2627123002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_passwords_section_browsertest.js:39: this.passwordManager = new TestPasswordManager(); Some drive by questions: Can we use the Mocha suite()'s setup and teardown instead of this one? This would simplify things by eliminating the need to do var self = this; as follows function helperFunction1(passwordManager) { //setup password manager here. } function validatePasswordList(...) { // helpers as top-level functions, not methods. } suite('mytestsuite', function() { var passwordManager = null; setup(function() { passwordManager = .....; }); test('mytest', function() { // refer to passwordManager here, or helper functions. }); }); This is also more robust since a new PasswordManager is created between Mocha test cases (no need to cleanup after previous PasswordManager). https://codereview.chromium.org/2627123002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_passwords_section_browsertest.js:58: validatePasswordList: function(listElement, passwordList) { A lot of the methods on SettingsPasswordSectionBrowserTest's prototype do not need any access to |this|. Should we just convert them to top-level functions instead of methods (related to previous comments code snippet). https://codereview.chromium.org/2627123002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_passwords_section_browsertest.js:260: self.passwordManager.onRemoveSavedPassword = function(detail) { The PasswordManager class seems to be just like any other "browser proxy". Can you use TestBrowserProxy to intercept calls to the browser instead? I would also suggest renaming PasswordManager to PasswordBrowserProxy or something like that, given that it calls chrome.* APIs.
what's the status of this CL?
The CQ bit was checked by hcarmona@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: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by hcarmona@chromium.org to run a CQ dry run
Patchset #5 (id:120001) 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...
Sorry this took a while. Updated w/ feedback + rebased. https://codereview.chromium.org/2627123002/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/settings/settings_passwords_section_browsertest.js (right): https://codereview.chromium.org/2627123002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_passwords_section_browsertest.js:39: this.passwordManager = new TestPasswordManager(); On 2017/01/28 00:10:30, dpapad wrote: > Some drive by questions: Can we use the Mocha suite()'s setup and teardown > instead of this one? This would simplify things by eliminating the need to do > var self = this; as follows > > function helperFunction1(passwordManager) { > //setup password manager here. > } > > function validatePasswordList(...) { > // helpers as top-level functions, not methods. > } > > suite('mytestsuite', function() { > var passwordManager = null; > > setup(function() { > passwordManager = .....; > }); > > test('mytest', function() { > // refer to passwordManager here, or helper functions. > }); > }); > > This is also more robust since a new PasswordManager is created between Mocha > test cases (no need to cleanup after previous PasswordManager). Awesome! https://codereview.chromium.org/2627123002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_passwords_section_browsertest.js:47: PasswordManagerImpl.instance_ = null; On 2017/01/27 23:55:22, Dan Beam wrote: > On 2017/01/27 18:28:16, hcarmona wrote: > > On 2017/01/27 01:11:32, Dan Beam wrote: > > > On 2017/01/26 21:34:47, hcarmona wrote: > > > > Moved this here to clean up after the test. > > > > > > we're just re-creating on setup, why does this matter? > > > > PasswordManagerImpl.instance_ is global. If 2 tests rely on this and it's not > > reset, then we can get into a state where one test will depend on the other. > > This leads to weird failures that depend on test order. > > i mean, you can leave this in, but nobody else does it ;) > > overriding a global instance: > https://cs.chromium.org/search/?q=%22instance_+%3D+%22+chrome/test/data/webui... > > tearing down a global instance: > https://cs.chromium.org/search/?q=%22instance_+%3D+null%22+chrome/test/data/w... Going w/ dpapad@'s suggestion for Mocha suite setup and removing the teardown since it doesn't make sense to keep it. Tests that need it will override it.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm w/nits i like the expectations thing as well https://codereview.chromium.org/2627123002/diff/140001/chrome/browser/resourc... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js (right): https://codereview.chromium.org/2627123002/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js:370: this.passwordManager_.getPlaintextPassword(event.detail, function(e) { maybe use a different param name than |e| here? https://codereview.chromium.org/2627123002/diff/140001/chrome/test/data/webui... File chrome/test/data/webui/settings/passwords_and_autofill_fake_data.js (right): https://codereview.chromium.org/2627123002/diff/140001/chrome/test/data/webui... chrome/test/data/webui/settings/passwords_and_autofill_fake_data.js:131: nit: /** @constructor */ https://codereview.chromium.org/2627123002/diff/140001/chrome/test/data/webui... chrome/test/data/webui/settings/passwords_and_autofill_fake_data.js:132: function PasswordManagerExpectations() {} nit: \n https://codereview.chromium.org/2627123002/diff/140001/chrome/test/data/webui... chrome/test/data/webui/settings/passwords_and_autofill_fake_data.js:158: } nit: \n https://codereview.chromium.org/2627123002/diff/140001/chrome/test/data/webui... chrome/test/data/webui/settings/passwords_and_autofill_fake_data.js:182: this.onRemoveSavedPassword(loginPair); ok, if these only exist in tests, that's fine i guess https://codereview.chromium.org/2627123002/diff/140001/chrome/test/data/webui... chrome/test/data/webui/settings/passwords_and_autofill_fake_data.js:269: } nit: \
The CQ bit was checked by hcarmona@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: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by hcarmona@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...
Fixed Nits + Rebased https://codereview.chromium.org/2627123002/diff/140001/chrome/browser/resourc... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js (right): https://codereview.chromium.org/2627123002/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js:370: this.passwordManager_.getPlaintextPassword(event.detail, function(e) { On 2017/02/14 06:18:06, Dan Beam wrote: > maybe use a different param name than |e| here? Done. https://codereview.chromium.org/2627123002/diff/140001/chrome/test/data/webui... File chrome/test/data/webui/settings/passwords_and_autofill_fake_data.js (right): https://codereview.chromium.org/2627123002/diff/140001/chrome/test/data/webui... chrome/test/data/webui/settings/passwords_and_autofill_fake_data.js:131: On 2017/02/14 06:18:06, Dan Beam wrote: > nit: /** @constructor */ Done. https://codereview.chromium.org/2627123002/diff/140001/chrome/test/data/webui... chrome/test/data/webui/settings/passwords_and_autofill_fake_data.js:132: function PasswordManagerExpectations() {} On 2017/02/14 06:18:06, Dan Beam wrote: > nit: \n Done. https://codereview.chromium.org/2627123002/diff/140001/chrome/test/data/webui... chrome/test/data/webui/settings/passwords_and_autofill_fake_data.js:158: } On 2017/02/14 06:18:06, Dan Beam wrote: > nit: \n Done. https://codereview.chromium.org/2627123002/diff/140001/chrome/test/data/webui... chrome/test/data/webui/settings/passwords_and_autofill_fake_data.js:182: this.onRemoveSavedPassword(loginPair); On 2017/02/14 06:18:06, Dan Beam wrote: > ok, if these only exist in tests, that's fine i guess Acknowledged. https://codereview.chromium.org/2627123002/diff/140001/chrome/test/data/webui... chrome/test/data/webui/settings/passwords_and_autofill_fake_data.js:269: } On 2017/02/14 06:18:06, Dan Beam wrote: > nit: \ Done.
https://codereview.chromium.org/2627123002/diff/180001/chrome/browser/resourc... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js (right): https://codereview.chromium.org/2627123002/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js:370: this.passwordManager_.getPlaintextPassword(event.detail, function(item) { Nit: The compiler does not know what type |event.detail| has here. Does this pass compilation? (I would not be surprised). Can we cast event.detail to the right type before passing? Also just to ensure that some type checking is happening, does this fail compilation if you pass something obviously wrong, for example getPlainTextPassword(34, function() {...}); https://codereview.chromium.org/2627123002/diff/180001/chrome/test/data/webui... File chrome/test/data/webui/settings/passwords_and_autofill_fake_data.js (right): https://codereview.chromium.org/2627123002/diff/180001/chrome/test/data/webui... chrome/test/data/webui/settings/passwords_and_autofill_fake_data.js:136: requested: { Why are those defined on the prototype? Can we moved them to the constructor instead? function PasswordManagerExpectations() { this.requested = ...; this.removed = ...; this.listening = ...; } https://codereview.chromium.org/2627123002/diff/180001/chrome/test/data/webui... chrome/test/data/webui/settings/passwords_and_autofill_fake_data.js:245: lastCallback: { Can |data| and |lastCallback| be declared in the constructor instead? From styleguide https://google.github.io/styleguide/jsguide.html#features-classes-fields, "Fields are never set on a concrete class' prototype." https://codereview.chromium.org/2627123002/diff/180001/chrome/test/data/webui... chrome/test/data/webui/settings/passwords_and_autofill_fake_data.js:254: requested: { Same here.
https://codereview.chromium.org/2627123002/diff/180001/chrome/test/data/webui... File chrome/test/data/webui/settings/passwords_and_autofill_fake_data.js (right): https://codereview.chromium.org/2627123002/diff/180001/chrome/test/data/webui... chrome/test/data/webui/settings/passwords_and_autofill_fake_data.js:136: requested: { On 2017/02/14 19:00:10, dpapad wrote: > Why are those defined on the prototype? Can we moved them to the constructor > instead? > > function PasswordManagerExpectations() { > this.requested = ...; > this.removed = ...; > this.listening = ...; > } fwiw, this is saner and safer
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 hcarmona@chromium.org to run a CQ dry run
https://codereview.chromium.org/2627123002/diff/180001/chrome/browser/resourc... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js (right): https://codereview.chromium.org/2627123002/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js:370: this.passwordManager_.getPlaintextPassword(event.detail, function(item) { On 2017/02/14 19:00:10, dpapad wrote: > Nit: The compiler does not know what type |event.detail| has here. Does this > pass compilation? (I would not be surprised). Can we cast event.detail to the > right type before passing? > > Also just to ensure that some type checking is happening, does this fail > compilation if you pass something obviously wrong, for example > getPlainTextPassword(34, function() {...}); Done. Required defining the passwordManager_ and fixing a few compile issues. https://codereview.chromium.org/2627123002/diff/180001/chrome/test/data/webui... File chrome/test/data/webui/settings/passwords_and_autofill_fake_data.js (right): https://codereview.chromium.org/2627123002/diff/180001/chrome/test/data/webui... chrome/test/data/webui/settings/passwords_and_autofill_fake_data.js:136: requested: { On 2017/02/14 19:33:23, Dan Beam wrote: > On 2017/02/14 19:00:10, dpapad wrote: > > Why are those defined on the prototype? Can we moved them to the constructor > > instead? > > > > function PasswordManagerExpectations() { > > this.requested = ...; > > this.removed = ...; > > this.listening = ...; > > } > > fwiw, this is saner and safer Done. Had to update |PasswordsAndFormsBrowserTest| for better setup. https://codereview.chromium.org/2627123002/diff/180001/chrome/test/data/webui... chrome/test/data/webui/settings/passwords_and_autofill_fake_data.js:245: lastCallback: { On 2017/02/14 19:00:10, dpapad wrote: > Can |data| and |lastCallback| be declared in the constructor instead? From > styleguide > https://google.github.io/styleguide/jsguide.html#features-classes-fields, > > "Fields are never set on a concrete class' prototype." Done. https://codereview.chromium.org/2627123002/diff/180001/chrome/test/data/webui... chrome/test/data/webui/settings/passwords_and_autofill_fake_data.js:254: requested: { On 2017/02/14 19:00:10, dpapad wrote: > Same here. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dpapad@chromium.org changed reviewers: + dpapad@chromium.org
https://codereview.chromium.org/2627123002/diff/200001/chrome/browser/resourc... File chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js (right): https://codereview.chromium.org/2627123002/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js:214: this.autofillManager_ = AutofillManagerImpl.getInstance(); Similar issue as with the password_section.js is probably happening here. Does the compiler even know what is the type of autofillManager_? It is not declared anywhere. We are missing a lot of compiler checks because of this. https://codereview.chromium.org/2627123002/diff/200001/chrome/browser/resourc... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js (right): https://codereview.chromium.org/2627123002/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js:242: this.setSavedPasswordsListener_); This is now a different reference than the one passed in addSavedPasswordListChangedListener(). Does the removal still work, or does it silently not remove the listener? I am guessing the latter. https://codereview.chromium.org/2627123002/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js:244: this.setPasswordExceptionsListener_); Same here. https://codereview.chromium.org/2627123002/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js:260: /** @type {function(!Array<!PasswordManager.PasswordUiEntry>):void} */ Why can't this be annotated like a normal member method? /** * @param {!Array<!PasswordManager.PasswordUiEntry>} list */ https://codereview.chromium.org/2627123002/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js:265: /** @type {function(!Array<!PasswordManager.ExceptionPair>):void} */ Same question here. https://codereview.chromium.org/2627123002/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js:373: this.setPassword(item.loginPair, item.plaintextPassword); Nit: Lines 373-374 should have +4 spaces of indentation.
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 hcarmona@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/2627123002/diff/200001/chrome/browser/resourc... File chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js (right): https://codereview.chromium.org/2627123002/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js:214: this.autofillManager_ = AutofillManagerImpl.getInstance(); On 2017/02/14 20:43:17, dpapad wrote: > Similar issue as with the password_section.js is probably happening here. Does > the compiler even know what is the type of autofillManager_? It is not declared > anywhere. We are missing a lot of compiler checks because of this. Done. https://codereview.chromium.org/2627123002/diff/200001/chrome/browser/resourc... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js (right): https://codereview.chromium.org/2627123002/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js:242: this.setSavedPasswordsListener_); On 2017/02/14 20:43:18, dpapad wrote: > This is now a different reference than the one passed in > addSavedPasswordListChangedListener(). Does the removal still work, or does it > silently not remove the listener? I am guessing the latter. Arg. Yes. Fixed. PTAL https://codereview.chromium.org/2627123002/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js:244: this.setPasswordExceptionsListener_); On 2017/02/14 20:43:18, dpapad wrote: > Same here. Acknowledged. https://codereview.chromium.org/2627123002/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js:260: /** @type {function(!Array<!PasswordManager.PasswordUiEntry>):void} */ On 2017/02/14 20:43:18, dpapad wrote: > Why can't this be annotated like a normal member method? > > /** > * @param {!Array<!PasswordManager.PasswordUiEntry>} list > */ Yes, that would have worked here, but had to go back to the old (current) way because it's a reference to a bound function. https://codereview.chromium.org/2627123002/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js:265: /** @type {function(!Array<!PasswordManager.ExceptionPair>):void} */ On 2017/02/14 20:43:18, dpapad wrote: > Same question here. Acknowledged. https://codereview.chromium.org/2627123002/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js:373: this.setPassword(item.loginPair, item.plaintextPassword); On 2017/02/14 20:43:18, dpapad wrote: > Nit: Lines 373-374 should have +4 spaces of indentation. Done.
LGTM with nits. https://codereview.chromium.org/2627123002/diff/220001/chrome/browser/resourc... File chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js (right): https://codereview.chromium.org/2627123002/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js:196: /** @type {AutofillManager} */ @private https://codereview.chromium.org/2627123002/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js:219: this.setAddressesListener_ = setAddressesListener; Nit: Are the local vars setAddressesListener and setCreditCardsListener necessary? Can we just inline as follows? this.setAddressesListener_ = function(list) { this.addresses = list; }.bind(this); this.setCreditCardsListener_ = function(list) { this.creditCards = list; }.bind(this); https://codereview.chromium.org/2627123002/diff/220001/chrome/browser/resourc... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js (right): https://codereview.chromium.org/2627123002/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js:112: getSavedPasswordList: function(callback) { Nit (for a potential future CL cleanup): Since we are already wrapping chrome.passwordsPrivate API, we could convert the old-style callback based API to Promise based. getSavedPasswordList: function() { return new Promise(function(resolve) { chrome.passwordsPrivate.getSavedPasswordList(resolve); }); }; Similarly for other methods in this class. https://codereview.chromium.org/2627123002/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js:238: this.setSavedPasswordsListener_ = setSavedPasswordsListener; Nit: Same question here as in autofill. Do we need the local vars setSavedPasswordsListener and setPasswordExceptionsListener?
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 hcarmona@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/2627123002/diff/220001/chrome/browser/resourc... File chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js (right): https://codereview.chromium.org/2627123002/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js:196: /** @type {AutofillManager} */ On 2017/02/15 02:01:38, dpapad wrote: > @private Done. Here, and elsewhere. https://codereview.chromium.org/2627123002/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js:219: this.setAddressesListener_ = setAddressesListener; On 2017/02/15 02:01:38, dpapad wrote: > Nit: Are the local vars setAddressesListener and setCreditCardsListener > necessary? Can we just inline as follows? > > this.setAddressesListener_ = function(list) { > this.addresses = list; > }.bind(this); > > this.setCreditCardsListener_ = function(list) { > this.creditCards = list; > }.bind(this); We can... but that will necessitate casting to the non-nullable type in all the calls. (just like in the |detached| method) https://codereview.chromium.org/2627123002/diff/220001/chrome/browser/resourc... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js (right): https://codereview.chromium.org/2627123002/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js:112: getSavedPasswordList: function(callback) { On 2017/02/15 02:01:38, dpapad wrote: > Nit (for a potential future CL cleanup): Since we are already wrapping > chrome.passwordsPrivate API, we could convert the old-style callback based API > to Promise based. > > getSavedPasswordList: function() { > return new Promise(function(resolve) { > chrome.passwordsPrivate.getSavedPasswordList(resolve); > }); > }; > > Similarly for other methods in this class. This wasn't done because the callback can occur async multiple times, not just when requested. https://codereview.chromium.org/2627123002/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js:238: this.setSavedPasswordsListener_ = setSavedPasswordsListener; On 2017/02/15 02:01:38, dpapad wrote: > Nit: Same question here as in autofill. Do we need the local vars > setSavedPasswordsListener and setPasswordExceptionsListener? Avoids type casting.
https://codereview.chromium.org/2627123002/diff/220001/chrome/browser/resourc... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js (right): https://codereview.chromium.org/2627123002/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js:112: getSavedPasswordList: function(callback) { On 2017/02/15 at 18:18:24, hcarmona wrote: > On 2017/02/15 02:01:38, dpapad wrote: > > Nit (for a potential future CL cleanup): Since we are already wrapping > > chrome.passwordsPrivate API, we could convert the old-style callback based API > > to Promise based. > > > > getSavedPasswordList: function() { > > return new Promise(function(resolve) { > > chrome.passwordsPrivate.t(resolve); > > }); > > }; > > > > Similarly for other methods in this class. > > This wasn't done because the callback can occur async multiple times, not just when requested. A single call to getSavedPasswordList() can invoke |callback| multiple times? That is surprising to me. The tests also don't seem to handle that case, https://cs.chromium.org/chromium/src/chrome/test/data/extensions/api_test/pas.... As well as the documentation of that method does not imply that it is called multiple times, https://cs.chromium.org/chromium/src/chrome/common/extensions/api/passwords_p.... Anyway, this was just a suggestion for a future cleanup, we can evaluate separately.
On 2017/02/15 18:32:21, dpapad wrote: > https://codereview.chromium.org/2627123002/diff/220001/chrome/browser/resourc... > File > chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js > (right): > > https://codereview.chromium.org/2627123002/diff/220001/chrome/browser/resourc... > chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js:112: > getSavedPasswordList: function(callback) { > On 2017/02/15 at 18:18:24, hcarmona wrote: > > On 2017/02/15 02:01:38, dpapad wrote: > > > Nit (for a potential future CL cleanup): Since we are already wrapping > > > chrome.passwordsPrivate API, we could convert the old-style callback based > API > > > to Promise based. > > > > > > getSavedPasswordList: function() { > > > return new Promise(function(resolve) { > > > chrome.passwordsPrivate.t(resolve); > > > }); > > > }; > > > > > > Similarly for other methods in this class. > > > > This wasn't done because the callback can occur async multiple times, not just > when requested. > > A single call to getSavedPasswordList() can invoke |callback| multiple times? > That is surprising to me. The tests also don't seem to handle that case, > https://cs.chromium.org/chromium/src/chrome/test/data/extensions/api_test/pas.... > As well as the documentation of that method does not imply that it is called > multiple times, > https://cs.chromium.org/chromium/src/chrome/common/extensions/api/passwords_p.... > > Anyway, this was just a suggestion for a future cleanup, we can evaluate > separately. You're right, I was remembering incorrectly. We use the same callback for addSavedPasswordListChangedListener and getSavedPasswordList. The listeners are called multiple times and getSavedPasswordList is called once.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Dan, just checking that this is still LG in your side, since we went through a fee revisions since you LG'd Thanks!
https://codereview.chromium.org/2627123002/diff/240001/chrome/browser/resourc... File chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js (right): https://codereview.chromium.org/2627123002/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js:115: chrome.autofillPrivate.removeEntry(guid); nit: assert() returns whatever it's passsed, so you can do chrome.autofillPrivate.removeEntry(assert(guid)); here and below https://codereview.chromium.org/2627123002/diff/240001/chrome/test/data/webui... File chrome/test/data/webui/settings/passwords_and_forms_browsertest.js (right): https://codereview.chromium.org/2627123002/diff/240001/chrome/test/data/webui... chrome/test/data/webui/settings/passwords_and_forms_browsertest.js:145: nit: remove \n https://codereview.chromium.org/2627123002/diff/240001/chrome/test/data/webui... File chrome/test/data/webui/settings/settings_passwords_section_browsertest.js (left): https://codereview.chromium.org/2627123002/diff/240001/chrome/test/data/webui... chrome/test/data/webui/settings/settings_passwords_section_browsertest.js:37: PolymerTest.clearBody(); why are you removing this?
The CQ bit was checked by hcarmona@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/2627123002/diff/240001/chrome/browser/resourc... File chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js (right): https://codereview.chromium.org/2627123002/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js:115: chrome.autofillPrivate.removeEntry(guid); On 2017/02/15 23:19:39, Dan Beam wrote: > nit: assert() returns whatever it's passsed, so you can do > > chrome.autofillPrivate.removeEntry(assert(guid)); > > here and below Done. https://codereview.chromium.org/2627123002/diff/240001/chrome/test/data/webui... File chrome/test/data/webui/settings/passwords_and_forms_browsertest.js (right): https://codereview.chromium.org/2627123002/diff/240001/chrome/test/data/webui... chrome/test/data/webui/settings/passwords_and_forms_browsertest.js:145: On 2017/02/15 23:19:39, Dan Beam wrote: > nit: remove \n Done. https://codereview.chromium.org/2627123002/diff/240001/chrome/test/data/webui... File chrome/test/data/webui/settings/settings_passwords_section_browsertest.js (left): https://codereview.chromium.org/2627123002/diff/240001/chrome/test/data/webui... chrome/test/data/webui/settings/settings_passwords_section_browsertest.js:37: PolymerTest.clearBody(); On 2017/02/15 23:19:39, Dan Beam wrote: > why are you removing this? Moved it to Mocha's setup so it runs between each test. Updated in the other tests to be consistent.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) closure_compilation on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
lgtm
The CQ bit was checked by hcarmona@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org Link to the patchset: https://codereview.chromium.org/2627123002/#ps260001 (title: "feedbavk")
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
Failed to apply patch for chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.html: While running git apply --index -p1; error: patch failed: chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.html:74 error: chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.html: patch does not apply Patch: chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.html Index: chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.html diff --git a/chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.html b/chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.html index 286491b0f1d5220dc64675b9a9ce72a3f6697fe5..06094ddc0d2a5a8202ac66693ef8829428012454 100644 --- a/chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.html +++ b/chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.html @@ -63,8 +63,7 @@ <settings-subpage associated-control="[[$$('#autofillManagerButton')]]" page-title="$i18n{autofill}"> - <settings-autofill-section id="autofillSection" - addresses="[[addresses]]" credit-cards="[[creditCards]]"> + <settings-autofill-section id="autofillSection"> </settings-autofill-section> </settings-subpage> </template> @@ -74,9 +73,8 @@ page-title="$i18n{passwords}" search-label="$i18n{searchPasswords}" search-term="{{passwordFilter_}}"> - <passwords-section saved-passwords="[[savedPasswords]]" - id="passwordSection" password-exceptions="[[passwordExceptions]]" - filter="[[passwordFilter_]]" prefs="{{prefs}}"> + <passwords-section id="passwordSection" filter="[[passwordFilter_]]" + prefs="{{prefs}}"> </passwords-section> </settings-subpage> </template>
The CQ bit was checked by hcarmona@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 hcarmona@chromium.org
The CQ bit was checked by hcarmona@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org, dpapad@chromium.org Link to the patchset: https://codereview.chromium.org/2627123002/#ps280001 (title: "rebase")
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
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by hcarmona@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 280001, "attempt_start_ts": 1487271995680850, "parent_rev": "80afbc06fd130755c7648d008bd6a9ddb8f65e16", "commit_rev": "7320f5e364e6c02b2d044612e4e488d70bb1e24f"}
Message was sent while issue was closed.
Description was changed from ========== Load Passwords and Autofill in the corresponding sub page. Moving the load into the sub page fixes the delay that is seen when navigating into the passwords sub page. This has the added advantage that items are not loaded at the top page. BUG=665748 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Load Passwords and Autofill in the corresponding sub page. Moving the load into the sub page fixes the delay that is seen when navigating into the passwords sub page. This has the added advantage that items are not loaded at the top page. BUG=665748 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2627123002 Cr-Commit-Position: refs/heads/master@{#451096} Committed: https://chromium.googlesource.com/chromium/src/+/7320f5e364e6c02b2d044612e4e4... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:280001) as https://chromium.googlesource.com/chromium/src/+/7320f5e364e6c02b2d044612e4e4... |