Closed
Bug 1147534
Opened 9 years ago
Closed 9 years ago
about:passwords is slow to load with lots of passwords
Categories
(Firefox for Android Graveyard :: Logins, Passwords and Form Fill, defect)
Tracking
(firefox40 fixed, relnote-firefox 40+, fennec40+)
RESOLVED
FIXED
Firefox 40
People
(Reporter: Margaret, Assigned: mfinkle)
References
Details
(Keywords: perf)
Attachments
(1 file)
5.07 KB,
patch
|
liuche
:
review+
|
Details | Diff | Splinter Review |
In my default profile, it can take multiple seconds for anything to appear on the screen when I load about:passwords. I know that on my desktop profile I have a few hundred logins stored, but I don't even see all of these appear in about:passwords.
Updated•9 years ago
|
tracking-fennec: ? → 40+
Reporter | ||
Updated•9 years ago
|
Assignee: nobody → liuche
Assignee | ||
Comment 1•9 years ago
|
||
I bet some of the slow performance is due to creating a new anonymous click handler for each list item: http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/aboutPasswords.js#132 We should make the | handleEvent | member function take care of the list item clicks by delegating to a | handleItemClick(event) helper |, and inside the helper we can use the | event | to get the | login | object attached to the list item. That way we avoid creating a JS function for all the list items.
Assignee | ||
Comment 2•9 years ago
|
||
This patch moves the "click" handler from an anonymous method, to delegated from the | handleEvent | method. There is only ever a single JS function now. I verified the existing popup menu still works as before. I'd like to see what affect this patch has on performance before moving on to something else.
Assignee: liuche → mark.finkle
Attachment #8588589 -
Flags: review?(liuche)
Reporter | ||
Comment 3•9 years ago
|
||
Comment on attachment 8588589 [details] [diff] [review] passwords-no-anonymous-click v0.1 Review of attachment 8588589 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/chrome/content/aboutPasswords.js @@ +179,5 @@ > > loginItem.setAttribute("loginID", login.guid); > loginItem.className = "login-item list-item"; > > + loginItem.addEventListener("click", this, true); Instead of adding a click listener to each individual item, you could also just add one to the entire list, and then check the target before handling the click.
Assignee | ||
Comment 4•9 years ago
|
||
(In reply to :Margaret Leibovic from comment #3) > > + loginItem.addEventListener("click", this, true); > > Instead of adding a click listener to each individual item, you could also > just add one to the entire list, and then check the target before handling > the click. Yeah, we could. I think we'd need to do a little dance, walking up the parent nodes, to get the "loginItem" div though. I don't know that the event has a target, currentTarget, originalTarget that would simply give the right element.
Comment 5•9 years ago
|
||
Comment on attachment 8588589 [details] [diff] [review] passwords-no-anonymous-click v0.1 Review of attachment 8588589 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/chrome/content/aboutPasswords.js @@ +179,5 @@ > > loginItem.setAttribute("loginID", login.guid); > loginItem.className = "login-item list-item"; > > + loginItem.addEventListener("click", this, true); I guess fewer event listeners is better? That sounds good to me.
Attachment #8588589 -
Flags: review?(liuche) → review+
Assignee | ||
Comment 6•9 years ago
|
||
I noticed that _loadList clones the main list element and then replaces it, so I did not try a single global click handler yet. https://hg.mozilla.org/integration/fx-team/rev/7150312e33c9
Comment 7•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/7150312e33c9
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Comment 8•9 years ago
|
||
Release Note Request (optional, but appreciated) [Why is this notable]: Nice performance improvement for users [Suggested wording]: About:password page now loads more quickly [Links (documentation, blog post, etc)]:
relnote-firefox:
--- → 40+
Comment 9•9 years ago
|
||
This should not be a relnote the code is not on Firefox 40 due to build flags. https://dxr.mozilla.org/mozilla-central/source/mobile/android/components/AboutRedirector.js#90
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•