Closed Bug 1147534 Opened 7 years ago Closed 7 years ago

about:passwords is slow to load with lots of passwords

Categories

(Firefox for Android Graveyard :: Logins, Passwords and Form Fill, defect)

35 Branch
All
Android
defect
Not set
normal

Tracking

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

RESOLVED FIXED
Firefox 40
Tracking Status
firefox40 --- fixed
relnote-firefox --- 40+
fennec 40+ ---

People

(Reporter: Margaret, Assigned: mfinkle)

References

Details

(Keywords: perf)

Attachments

(1 file)

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.
tracking-fennec: --- → ?
Keywords: perf
tracking-fennec: ? → 40+
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.
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)
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
Closed: 7 years ago
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)]:
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
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.