1147534 - about:passwords is slow to load with lots of passwords

Status: RESOLVED FIXED

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

Description

[:Margaret Leibovic]

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.

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: 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.

Comment 3

[:Margaret Leibovic]

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.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

(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

[Chenxia Liu [: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);

I guess fewer event listeners is better? That sounds good to me.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

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

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7150312e33c9

Comment 8

[Liz Henry (:lizzard) (relman/hg->git project)]

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)]:

Comment 9

[Kevin Brosnan [Ex-Mozilla]]

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