Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(18)

Issue 2627123002: Load Passwords and Autofill in the corresponding sub page. (Closed)

Created:
3 years, 11 months ago by hcarmona
Modified:
3 years, 10 months ago
Reviewers:
Dan Beam, dpapad
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.

Description

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/+/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)
hcarmona
Sorry for big patch! PTAL. Most changes are moving code around. Had to update tests ...
3 years, 11 months ago (2017-01-23 19:16:20 UTC) #17
Dan Beam
looks pretty good! https://codereview.chromium.org/2627123002/diff/60001/chrome/test/data/webui/settings/passwords_and_autofill_fake_data.js File chrome/test/data/webui/settings/passwords_and_autofill_fake_data.js (right): https://codereview.chromium.org/2627123002/diff/60001/chrome/test/data/webui/settings/passwords_and_autofill_fake_data.js#newcode132 chrome/test/data/webui/settings/passwords_and_autofill_fake_data.js:132: function PasswordManagerExpectations() {}; don't put a ...
3 years, 10 months ago (2017-01-26 19:10:45 UTC) #20
hcarmona
https://codereview.chromium.org/2627123002/diff/60001/chrome/test/data/webui/settings/passwords_and_autofill_fake_data.js File chrome/test/data/webui/settings/passwords_and_autofill_fake_data.js (right): https://codereview.chromium.org/2627123002/diff/60001/chrome/test/data/webui/settings/passwords_and_autofill_fake_data.js#newcode132 chrome/test/data/webui/settings/passwords_and_autofill_fake_data.js:132: function PasswordManagerExpectations() {}; On 2017/01/26 19:10:44, Dan Beam wrote: ...
3 years, 10 months ago (2017-01-26 21:34:48 UTC) #23
Dan Beam
https://codereview.chromium.org/2627123002/diff/60001/chrome/test/data/webui/settings/passwords_and_forms_browsertest.js File chrome/test/data/webui/settings/passwords_and_forms_browsertest.js (right): https://codereview.chromium.org/2627123002/diff/60001/chrome/test/data/webui/settings/passwords_and_forms_browsertest.js#newcode66 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: > ...
3 years, 10 months ago (2017-01-27 01:11:33 UTC) #26
hcarmona
https://codereview.chromium.org/2627123002/diff/60001/chrome/test/data/webui/settings/passwords_and_forms_browsertest.js File chrome/test/data/webui/settings/passwords_and_forms_browsertest.js (right): https://codereview.chromium.org/2627123002/diff/60001/chrome/test/data/webui/settings/passwords_and_forms_browsertest.js#newcode66 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: ...
3 years, 10 months ago (2017-01-27 18:28:16 UTC) #29
Dan Beam
still thinking about your onRemoveSavedPassword stuff... https://codereview.chromium.org/2627123002/diff/80001/chrome/test/data/webui/settings/settings_passwords_section_browsertest.js File chrome/test/data/webui/settings/settings_passwords_section_browsertest.js (right): https://codereview.chromium.org/2627123002/diff/80001/chrome/test/data/webui/settings/settings_passwords_section_browsertest.js#newcode47 chrome/test/data/webui/settings/settings_passwords_section_browsertest.js:47: PasswordManagerImpl.instance_ = null; ...
3 years, 10 months ago (2017-01-27 23:55:22 UTC) #32
dpapad
https://codereview.chromium.org/2627123002/diff/80001/chrome/test/data/webui/settings/settings_passwords_section_browsertest.js File chrome/test/data/webui/settings/settings_passwords_section_browsertest.js (right): https://codereview.chromium.org/2627123002/diff/80001/chrome/test/data/webui/settings/settings_passwords_section_browsertest.js#newcode39 chrome/test/data/webui/settings/settings_passwords_section_browsertest.js:39: this.passwordManager = new TestPasswordManager(); Some drive by questions: Can ...
3 years, 10 months ago (2017-01-28 00:10:30 UTC) #33
Dan Beam
what's the status of this CL?
3 years, 10 months ago (2017-02-09 23:51:53 UTC) #34
hcarmona
Sorry this took a while. Updated w/ feedback + rebased. https://codereview.chromium.org/2627123002/diff/80001/chrome/test/data/webui/settings/settings_passwords_section_browsertest.js File chrome/test/data/webui/settings/settings_passwords_section_browsertest.js (right): https://codereview.chromium.org/2627123002/diff/80001/chrome/test/data/webui/settings/settings_passwords_section_browsertest.js#newcode39 ...
3 years, 10 months ago (2017-02-14 00:59:38 UTC) #42
Dan Beam
lgtm w/nits i like the expectations thing as well https://codereview.chromium.org/2627123002/diff/140001/chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js File chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js (right): https://codereview.chromium.org/2627123002/diff/140001/chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js#newcode370 chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js:370: ...
3 years, 10 months ago (2017-02-14 06:18:06 UTC) #45
hcarmona
Fixed Nits + Rebased https://codereview.chromium.org/2627123002/diff/140001/chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js File chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js (right): https://codereview.chromium.org/2627123002/diff/140001/chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js#newcode370 chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js:370: this.passwordManager_.getPlaintextPassword(event.detail, function(e) { On 2017/02/14 ...
3 years, 10 months ago (2017-02-14 18:38:44 UTC) #52
dpapad
https://codereview.chromium.org/2627123002/diff/180001/chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js File chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js (right): https://codereview.chromium.org/2627123002/diff/180001/chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js#newcode370 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 ...
3 years, 10 months ago (2017-02-14 19:00:10 UTC) #53
Dan Beam
https://codereview.chromium.org/2627123002/diff/180001/chrome/test/data/webui/settings/passwords_and_autofill_fake_data.js File chrome/test/data/webui/settings/passwords_and_autofill_fake_data.js (right): https://codereview.chromium.org/2627123002/diff/180001/chrome/test/data/webui/settings/passwords_and_autofill_fake_data.js#newcode136 chrome/test/data/webui/settings/passwords_and_autofill_fake_data.js:136: requested: { On 2017/02/14 19:00:10, dpapad wrote: > Why ...
3 years, 10 months ago (2017-02-14 19:33:23 UTC) #54
hcarmona
https://codereview.chromium.org/2627123002/diff/180001/chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js File chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js (right): https://codereview.chromium.org/2627123002/diff/180001/chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js#newcode370 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: > ...
3 years, 10 months ago (2017-02-14 20:34:30 UTC) #58
dpapad
https://codereview.chromium.org/2627123002/diff/200001/chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js File chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js (right): https://codereview.chromium.org/2627123002/diff/200001/chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js#newcode214 chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js:214: this.autofillManager_ = AutofillManagerImpl.getInstance(); Similar issue as with the password_section.js ...
3 years, 10 months ago (2017-02-14 20:43:18 UTC) #61
hcarmona
https://codereview.chromium.org/2627123002/diff/200001/chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js File chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js (right): https://codereview.chromium.org/2627123002/diff/200001/chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js#newcode214 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: > ...
3 years, 10 months ago (2017-02-15 00:13:35 UTC) #66
dpapad
LGTM with nits. https://codereview.chromium.org/2627123002/diff/220001/chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js File chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js (right): https://codereview.chromium.org/2627123002/diff/220001/chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js#newcode196 chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js:196: /** @type {AutofillManager} */ @private https://codereview.chromium.org/2627123002/diff/220001/chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js#newcode219 ...
3 years, 10 months ago (2017-02-15 02:01:38 UTC) #67
hcarmona
https://codereview.chromium.org/2627123002/diff/220001/chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js File chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js (right): https://codereview.chromium.org/2627123002/diff/220001/chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js#newcode196 chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js:196: /** @type {AutofillManager} */ On 2017/02/15 02:01:38, dpapad wrote: ...
3 years, 10 months ago (2017-02-15 18:18:24 UTC) #72
dpapad
https://codereview.chromium.org/2627123002/diff/220001/chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js File chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js (right): https://codereview.chromium.org/2627123002/diff/220001/chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js#newcode112 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: ...
3 years, 10 months ago (2017-02-15 18:32:21 UTC) #73
hcarmona
On 2017/02/15 18:32:21, dpapad wrote: > https://codereview.chromium.org/2627123002/diff/220001/chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js > File > chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js > (right): > > ...
3 years, 10 months ago (2017-02-15 18:38:42 UTC) #74
hcarmona
Dan, just checking that this is still LG in your side, since we went through ...
3 years, 10 months ago (2017-02-15 23:11:24 UTC) #77
Dan Beam
https://codereview.chromium.org/2627123002/diff/240001/chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js File chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js (right): https://codereview.chromium.org/2627123002/diff/240001/chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js#newcode115 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 ...
3 years, 10 months ago (2017-02-15 23:19:39 UTC) #78
hcarmona
https://codereview.chromium.org/2627123002/diff/240001/chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js File chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js (right): https://codereview.chromium.org/2627123002/diff/240001/chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js#newcode115 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: ...
3 years, 10 months ago (2017-02-16 00:00:01 UTC) #81
Dan Beam
lgtm
3 years, 10 months ago (2017-02-16 03:40:47 UTC) #84
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2627123002/260001
3 years, 10 months ago (2017-02-16 17:34:47 UTC) #87
commit-bot: I haz the power
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: ...
3 years, 10 months ago (2017-02-16 17:43:34 UTC) #89
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2627123002/280001
3 years, 10 months ago (2017-02-16 17:49:51 UTC) #95
commit-bot: I haz the power
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_swarming_rel/builds/120641) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, ...
3 years, 10 months ago (2017-02-16 17:56:05 UTC) #97
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2627123002/280001
3 years, 10 months ago (2017-02-16 19:07:34 UTC) #99
commit-bot: I haz the power
3 years, 10 months ago (2017-02-16 21:08:48 UTC) #102
Message was sent while issue was closed.
Committed patchset #12 (id:280001) as
https://chromium.googlesource.com/chromium/src/+/7320f5e364e6c02b2d044612e4e4...

Powered by Google App Engine
This is Rietveld 408576698