about:passwords is slow to load with lots of passwords

RESOLVED FIXED in Firefox 40

Status

()

Firefox for Android
Logins, Passwords and Form Fill
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Margaret, Assigned: mfinkle)

Tracking

(Blocks: 1 bug, {perf})

35 Branch
Firefox 40
All
Android
Points:
---

Firefox Tracking Flags

(firefox40 fixed, relnote-firefox 40+, fennec40+)

Details

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
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

3 years ago
tracking-fennec: --- → ?
Keywords: perf
tracking-fennec: ? → 40+
(Reporter)

Updated

3 years ago
Assignee: nobody → liuche
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.
Created attachment 8588589 [details] [diff] [review]
passwords-no-anonymous-click v0.1

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

3 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.
(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 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+
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
https://hg.mozilla.org/mozilla-central/rev/7150312e33c9
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
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+
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
You need to log in before you can comment on or make changes to this bug.