Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2254893012:
Give the password list a CrScrollableBehavior to allow refresh to work. (Closed)
|
Created:
4 years, 4 months ago by hcarmona Modified:
4 years, 4 months ago CC:
chromium-reviews, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, arv+watch_chromium.org, vitalyp+closure_chromium.org, dbeam+watch-settings_chromium.org, dbeam+watch-closure_chromium.org, stevenjb+watch-md-settings_chromium.org, jlklein+watch-closure_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionGive the password list a CrScrollableBehavior to allow refresh to work.
BUG=600812
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/f8f406b633bfad5f28d1f532a643e1ae6da2b012
Cr-Commit-Position: refs/heads/master@{#413548}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Use CrScrollableBehavior #
Total comments: 2
Patch Set 3 : use an observer #
Messages
Total messages: 29 (20 generated)
Description was changed from ========== Notify the list of passwords that it has been sized after rendering. BUG=600812 ========== to ========== Notify the list of passwords that it has been sized after rendering. BUG=600812 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...
hcarmona@chromium.org changed reviewers: + dschuyler@chromium.org, stevenjb@chromium.org
After what seems like forever, trying to figure out when to |notifyResize| the list, I found |afterNextRender|. PTAL
https://codereview.chromium.org/2254893012/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js (right): https://codereview.chromium.org/2254893012/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js:75: Polymer.RenderStatus.afterNextRender(this, function() { Interesting. Would be nice if this were documented on the Polymer site. That said, I tried using this for CrScrollableBehavior (https://codereview.chromium.org/2256753003/) but it did not do the trick, whereas setTimeout or setInterval did. We probably want to use CrScrollableBehavior here, in which case we can just call updateScrollableContents() from a (savedPasswords, filter) observer.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
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 checked by hcarmona@chromium.org to run a CQ dry run
Patchset #2 (id:20001) 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 ========== Notify the list of passwords that it has been sized after rendering. BUG=600812 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Give the password list a CrScrollableBehavior to allow refresh to work. BUG=600812 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
https://codereview.chromium.org/2254893012/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js (right): https://codereview.chromium.org/2254893012/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js:75: Polymer.RenderStatus.afterNextRender(this, function() { On 2016/08/18 19:26:32, stevenjb wrote: > Interesting. Would be nice if this were documented on the Polymer site. > > That said, I tried using this for CrScrollableBehavior > (https://codereview.chromium.org/2256753003/) but it did not do the trick, > whereas setTimeout or setInterval did. > > We probably want to use CrScrollableBehavior here, in which case we can just > call updateScrollableContents() from a (savedPasswords, filter) observer. I've changed this to use CrScrollableBehavior for consistency.
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...)
Thanks for using CrScrollableBehavior! https://codereview.chromium.org/2254893012/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js (right): https://codereview.chromium.org/2254893012/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js:78: this.updateScrollableContents(); This may be working now, but if the timing changes (i.e. attached gets called before savedPasswords gets set) it might break. It could also break if the list of saved passwords changes from short or empty to very long (e.g. after sync). I would recommend instead adding: observers: [ 'passwordListChanged_(savedPasswords, filte)', ], passwordListChanged_: function() { this.updateScrollableContents(); }
The CQ bit was checked by hcarmona@chromium.org to run a CQ dry run
https://codereview.chromium.org/2254893012/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js (right): https://codereview.chromium.org/2254893012/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js:78: this.updateScrollableContents(); On 2016/08/22 16:25:56, stevenjb wrote: > This may be working now, but if the timing changes (i.e. attached gets called > before savedPasswords gets set) it might break. It could also break if the list > of saved passwords changes from short or empty to very long (e.g. after sync). I > would recommend instead adding: > > observers: [ > 'passwordListChanged_(savedPasswords, filte)', > ], > > passwordListChanged_: function() { this.updateScrollableContents(); } > > Done.
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 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 dschuyler@chromium.org Link to the patchset: https://codereview.chromium.org/2254893012/#ps60001 (title: "use an observer")
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 ========== Give the password list a CrScrollableBehavior to allow refresh to work. BUG=600812 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Give the password list a CrScrollableBehavior to allow refresh to work. BUG=600812 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/f8f406b633bfad5f28d1f532a643e1ae6da2b012 Cr-Commit-Position: refs/heads/master@{#413548} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/f8f406b633bfad5f28d1f532a643e1ae6da2b012 Cr-Commit-Position: refs/heads/master@{#413548} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
