Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2571443002:
Fix bug when transitioning from an empty to a non-empty password list (Closed)
|
Created:
4 years ago by jdoerrie Modified:
4 years ago CC:
chromium-reviews, dbeam+watch-options_chromium.org, gcasto+watchlist_chromium.org, michaelpg+watch-options_chromium.org, arv+watch_chromium.org, vabr+watchlistpasswordmanager_chromium.org, kolos1 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix bug when transitioning from an empty to a non-empty password list
This change fixes a bug that occurs when a list transitions from an empty to a
non-empty state. Before this change, the entries of the list were updated before
its visibility, which causes |List.redraw()| to skip adding items to the
viewport when the list was hidden. Now the visibility will be updated first,
leading to correct behavior.
BUG=672869
R=dbeam@chromium.org
CC=kolos@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/0edd353c07b8cebe483ec64f5e1abe638001d2d4
Cr-Commit-Position: refs/heads/master@{#440048}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Fix Closure Compiler Errors #
Total comments: 2
Patch Set 3 : Updated comment for clarity #
Total comments: 2
Patch Set 4 : Rebase and inline assert #Messages
Total messages: 29 (17 generated)
Description was changed from ========== Fix bug when transitioning from an empty to a non-empty password list This change fixes a bug that occurs when a list transitions from an empty to a non-empty state. Before this change, the entries of the list were updated before its visibility, which causes a bug due to the lazy nature of the list's dataModel setter. Now the visibility will be updated first, leading to correct behaviour. BUG=672869 R=dbeam@chromium.org CC=kolos@chromium.org ========== to ========== Fix bug when transitioning from an empty to a non-empty password list This change fixes a bug that occurs when a list transitions from an empty to a non-empty state. Before this change, the entries of the list were updated before its visibility, which causes a bug due to the lazy nature of the list's dataModel setter. Now the visibility will be updated first, leading to correct behaviour. BUG=672869 R=dbeam@chromium.org CC=kolos@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by jdoerrie@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...
Dear all, PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: closure_compilation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)
kolos@chromium.org changed reviewers: + kolos@chromium.org
https://codereview.chromium.org/2571443002/diff/1/chrome/browser/resources/op... File chrome/browser/resources/options/password_manager.js (right): https://codereview.chromium.org/2571443002/diff/1/chrome/browser/resources/op... chrome/browser/resources/options/password_manager.js:256: this.updateListAndVisibility_(this.savedPasswordsList_, entries); To prevent closure compilation error, we could add "if" to check whether this.savedPasswordsList_ is null or not. If no, call updateListAndVisibility_ and updateOriginsEliding_.
The CQ bit was checked by jdoerrie@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.
https://codereview.chromium.org/2571443002/diff/20001/chrome/browser/resource... File chrome/browser/resources/options/password_manager.js (right): https://codereview.chromium.org/2571443002/diff/20001/chrome/browser/resource... chrome/browser/resources/options/password_manager.js:169: // Since the dataModel setter behaves lazily, the visibility needs to be i'm confused. ListItem#set dataModel() seems to be synchronous https://cs.chromium.org/chromium/src/ui/webui/resources/js/cr/ui/list.js?q=se... and so does the ctor initialization of ArrayDataModel and get length(): https://cs.chromium.org/chromium/src/ui/webui/resources/js/cr/ui/array_data_m...
Description was changed from ========== Fix bug when transitioning from an empty to a non-empty password list This change fixes a bug that occurs when a list transitions from an empty to a non-empty state. Before this change, the entries of the list were updated before its visibility, which causes a bug due to the lazy nature of the list's dataModel setter. Now the visibility will be updated first, leading to correct behaviour. BUG=672869 R=dbeam@chromium.org CC=kolos@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Fix bug when transitioning from an empty to a non-empty password list This change fixes a bug that occurs when a list transitions from an empty to a non-empty state. Before this change, the entries of the list were updated before its visibility, which causes |List.redraw()| to skip adding items to the viewport when the list was hidden. Now the visibility will be updated first, leading to correct behaviour. BUG=672869 R=dbeam@chromium.org CC=kolos@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Fix bug when transitioning from an empty to a non-empty password list This change fixes a bug that occurs when a list transitions from an empty to a non-empty state. Before this change, the entries of the list were updated before its visibility, which causes |List.redraw()| to skip adding items to the viewport when the list was hidden. Now the visibility will be updated first, leading to correct behaviour. BUG=672869 R=dbeam@chromium.org CC=kolos@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Fix bug when transitioning from an empty to a non-empty password list This change fixes a bug that occurs when a list transitions from an empty to a non-empty state. Before this change, the entries of the list were updated before its visibility, which causes |List.redraw()| to skip adding items to the viewport when the list was hidden. Now the visibility will be updated first, leading to correct behavior. BUG=672869 R=dbeam@chromium.org CC=kolos@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
https://codereview.chromium.org/2571443002/diff/20001/chrome/browser/resource... File chrome/browser/resources/options/password_manager.js (right): https://codereview.chromium.org/2571443002/diff/20001/chrome/browser/resource... chrome/browser/resources/options/password_manager.js:169: // Since the dataModel setter behaves lazily, the visibility needs to be On 2016/12/13 02:11:16, Dan Beam wrote: > i'm confused. ListItem#set dataModel() seems to be synchronous > > https://cs.chromium.org/chromium/src/ui/webui/resources/js/cr/ui/list.js?q=se... > > and so does the ctor initialization of ArrayDataModel and get length(): > > https://cs.chromium.org/chromium/src/ui/webui/resources/js/cr/ui/array_data_m... I should have expressed myself more clearly. The culprit is the following line in List.redraw(): https://cs.chromium.org/chromium/src/ui/webui/resources/js/cr/ui/list.js?l=1062 this.clientHeight will be 0 when list.hidden == true, thus no items will be added to the list and redraw returns early in line 1068. redraw is called in the dataModel setter: https://cs.chromium.org/chromium/src/ui/webui/resources/js/cr/ui/list.js?l=149 Line 1062 also explains why this bug only occurs in M55 and later, |autoExpands_| was set to true before I landed http://crrev.com/2443143002. With |autoExpands = true|, the condition in the if statement evaluated to false, rendering the list items properly. I updated the comment in |updateListAndVisibility_| and the issue description to be more precise.
can we just set autoexpands = true? https://cs.chromium.org/chromium/src/ui/webui/resources/js/cr/ui/list.js?q=au...
On 2016/12/13 19:54:48, Dan Beam wrote: > can we just set autoexpands = true? > > https://cs.chromium.org/chromium/src/ui/webui/resources/js/cr/ui/list.js?q=au... Unfortunately we can't. Removing |autoExpands = true| was key to solve the performance issues in http://crbug.com/651049. If it is set to true, |list.redraw()| updates O(|dataModel.length|) DOM elements (https://cs.chromium.org/chromium/src/ui/webui/resources/js/cr/ui/list.js?q=au...), which can be very slow for large lists such as passwords or user defined dictionaries (https://cs.chromium.org/chromium/src/chrome/browser/resources/options/languag...). This is why autoExpands is disabled in these cases.
On 2016/12/14 16:56:03, jdoerrie wrote: > On 2016/12/13 19:54:48, Dan Beam wrote: > > can we just set autoexpands = true? > > > > > https://cs.chromium.org/chromium/src/ui/webui/resources/js/cr/ui/list.js?q=au... > > Unfortunately we can't. Removing |autoExpands = true| was key to solve the > performance issues in http://crbug.com/651049. If it is set to true, > |list.redraw()| updates O(|dataModel.length|) DOM elements > (https://cs.chromium.org/chromium/src/ui/webui/resources/js/cr/ui/list.js?q=au...), > which can be very slow for large lists such as passwords or user defined > dictionaries > (https://cs.chromium.org/chromium/src/chrome/browser/resources/options/languag...). > This is why autoExpands is disabled in these cases. Friendly ping on this one. Changing the order of setting visibility and dataModel seems to be the only way to avoid this bug from happening without sacrificing a lot of performance.
lgtm https://codereview.chromium.org/2571443002/diff/40001/chrome/browser/resource... File chrome/browser/resources/options/password_manager.js (right): https://codereview.chromium.org/2571443002/diff/40001/chrome/browser/resource... chrome/browser/resources/options/password_manager.js:259: this.updateListAndVisibility_(this.savedPasswordsList_, entries); can you just wrap with assert(), as in: this.updateListAndVisibility_(assert(this.savedPasswordsList_), entries);
https://codereview.chromium.org/2571443002/diff/40001/chrome/browser/resource... File chrome/browser/resources/options/password_manager.js (right): https://codereview.chromium.org/2571443002/diff/40001/chrome/browser/resource... chrome/browser/resources/options/password_manager.js:259: this.updateListAndVisibility_(this.savedPasswordsList_, entries); On 2016/12/20 22:25:12, Dan Beam wrote: > can you just wrap with assert(), as in: > > this.updateListAndVisibility_(assert(this.savedPasswordsList_), entries); Done.
The CQ bit was checked by jdoerrie@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/2571443002/#ps60001 (title: "Rebase and inline assert")
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": 60001, "attempt_start_ts": 1482304069371080,
"parent_rev": "2423c2633761a762f31a649b11c18875b515b271", "commit_rev":
"e2fb22a6a4b5c90372e410567f288836eb37ff7f"}
Message was sent while issue was closed.
Description was changed from ========== Fix bug when transitioning from an empty to a non-empty password list This change fixes a bug that occurs when a list transitions from an empty to a non-empty state. Before this change, the entries of the list were updated before its visibility, which causes |List.redraw()| to skip adding items to the viewport when the list was hidden. Now the visibility will be updated first, leading to correct behavior. BUG=672869 R=dbeam@chromium.org CC=kolos@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Fix bug when transitioning from an empty to a non-empty password list This change fixes a bug that occurs when a list transitions from an empty to a non-empty state. Before this change, the entries of the list were updated before its visibility, which causes |List.redraw()| to skip adding items to the viewport when the list was hidden. Now the visibility will be updated first, leading to correct behavior. BUG=672869 R=dbeam@chromium.org CC=kolos@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2571443002 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Fix bug when transitioning from an empty to a non-empty password list This change fixes a bug that occurs when a list transitions from an empty to a non-empty state. Before this change, the entries of the list were updated before its visibility, which causes |List.redraw()| to skip adding items to the viewport when the list was hidden. Now the visibility will be updated first, leading to correct behavior. BUG=672869 R=dbeam@chromium.org CC=kolos@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2571443002 ========== to ========== Fix bug when transitioning from an empty to a non-empty password list This change fixes a bug that occurs when a list transitions from an empty to a non-empty state. Before this change, the entries of the list were updated before its visibility, which causes |List.redraw()| to skip adding items to the viewport when the list was hidden. Now the visibility will be updated first, leading to correct behavior. BUG=672869 R=dbeam@chromium.org CC=kolos@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/0edd353c07b8cebe483ec64f5e1abe638001d2d4 Cr-Commit-Position: refs/heads/master@{#440048} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/0edd353c07b8cebe483ec64f5e1abe638001d2d4 Cr-Commit-Position: refs/heads/master@{#440048} |
