735636 - PromptService list view has bad scroll performance

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Lucas Rocha (:lucasr)]

It should be using convertView correctly (avoids inflating every row) and using a viewholder pattern to avoid calling findViewById too often.

Comment 1

[Martijn Wargers (dead)]

You mean select for helper popups like this one: http://people.mozilla.org/~mwargers/tests/forms/select/selectoptions_alot.html

Comment 2

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

How bad is scroll performance? How big of a list?

Comment 3

[Martijn Wargers (dead)]

On the Asus Transformer TF101 tablet, I've seen it causing the "Appliction doesn not respond, close it?" dialog to appear, while scrolling in the select form helper popup on the testcase.

Comment 4

[Lucas Rocha (:lucasr)]

(In reply to Mark Finkle (:mfinkle) from comment #2)
> How bad is scroll performance? How big of a list?

It's not necessarily related to the size of the list. Basically, ListView recycles a lot of views while scrolling and the ListView adapter used in PromptService is not using the recycled view (aka convertView) and inflating rows on every getView() call.

It should be straightforward to change PromptService to use convertView and ViewHolder pattern.

Comment 5

[Lucas Rocha (:lucasr)]

Attachment: (1/3) Correctly use recycled view and apply ViewHolder pattern

Comment 6

[Lucas Rocha (:lucasr)]

Attachment: (2/3) Factor out icon update into separate method

Comment 7

[Lucas Rocha (:lucasr)]

Attachment: (3/3) Factor out checked state update into separate method

Comment 8

[Wesley Johnston (:wesj)]

Comment on attachment 607945 [details] [diff] [review]
(1/3) Correctly use recycled view and apply ViewHolder pattern

Review of attachment 607945 [details] [diff] [review]:
-----------------------------------------------------------------

Sounds like (from irc), we need to tweak this a bit.

::: mobile/android/base/PromptService.java
@@ +490,5 @@
> +            if (convertView == null) {
> +                int resourceId = mResourceId;
> +                if (item.isGroup) {
> +                    resourceId = R.layout.list_item_header;
> +                }

Docs say "Note: You should check that this view is non-null and of an appropriate type before using." How do we know whether the convertView we get is a header row or not? I guess we need to inform the Adapter of how many row types we have? I'll throw up a demo with a lot of rows including some header ones.

Comment 9

[Wesley Johnston (:wesj)]

Attachment: Test case

This is just a long multiple select list with header rows scattered throughout.

Comment 10

[Wesley Johnston (:wesj)]

Comment on attachment 607947 [details] [diff] [review]
(3/3) Factor out checked state update into separate method

Review of attachment 607947 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/PromptService.java
@@ +509,5 @@
> +        private void maybeUpdateCheckedState(int position, PromptListItem item, TextView t) {
> +            if (item.isGroup)
> +                return;
> +
> +            CheckedTextView ct = (CheckedTextView) t;

I'm not sure all of these are checkedTextViews (i.e. context menus). Return if this is null, but I have a feeling it will actually throw instead and the gigantic try catch I had before was saving us, so we might be better to do a fancier check.... (I like instanceOf, but blassey tells me if you're using instanceOf, you're doing it wrong)

Comment 12

[Lucas Rocha (:lucasr)]

(In reply to Wesley Johnston (:wesj) from comment #10)
> Comment on attachment 607947 [details] [diff] [review]
> (3/3) Factor out checked state update into separate method
> 
> Review of attachment 607947 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/PromptService.java
> @@ +509,5 @@
> > +        private void maybeUpdateCheckedState(int position, PromptListItem item, TextView t) {
> > +            if (item.isGroup)
> > +                return;
> > +
> > +            CheckedTextView ct = (CheckedTextView) t;
> 
> I'm not sure all of these are checkedTextViews (i.e. context menus). Return
> if this is null, but I have a feeling it will actually throw instead and the
> gigantic try catch I had before was saving us, so we might be better to do a
> fancier check.... (I like instanceOf, but blassey tells me if you're using
> instanceOf, you're doing it wrong)

IIUC, the view will always be a CheckedTextView if it's not a group because we're using Android's select_dialog_multichoice and select_dialog_singlechoice layout.

Comment 13

[Lucas Rocha (:lucasr)]

Attachment: (1/3) Correctly use recycled view and apply ViewHolder pattern

Comment 14

[Lucas Rocha (:lucasr)]

Attachment: (2/3) Factor out icon update into separate method

Comment 15

[Lucas Rocha (:lucasr)]

Attachment: (3/3) Factor out checked state update into separate method

Comment 16

[Wesley Johnston (:wesj)]

Comment on attachment 610064 [details] [diff] [review]
(3/3) Factor out checked state update into separate method

Review of attachment 610064 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/PromptService.java
@@ +530,5 @@
> +        private void maybeUpdateCheckedState(int position, PromptListItem item, TextView t) {
> +            if (item.isGroup)
> +                return;
> +
> +            CheckedTextView ct = (CheckedTextView) t;

Context menus use android.R.layout.select_dialog_item which I don't think is a CheckedTextView.

Comment 17

[Lucas Rocha (:lucasr)]

Attachment: Factor out checked state update into separate method

Comment 18

[Wesley Johnston (:wesj)]

Make sure you've tested this with some different select type popups and context menus!

Comment 19

[Lucas Rocha (:lucasr)]

Pushed:
http://hg.mozilla.org/integration/mozilla-inbound/rev/96fcc34dbbc9
http://hg.mozilla.org/integration/mozilla-inbound/rev/bb20be085b35
http://hg.mozilla.org/integration/mozilla-inbound/rev/130e702908ca

Comment 20

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/96fcc34dbbc9
https://hg.mozilla.org/mozilla-central/rev/bb20be085b35
https://hg.mozilla.org/mozilla-central/rev/130e702908ca

Comment 21

[Lucas Rocha (:lucasr)]

Comment on attachment 610062 [details] [diff] [review]
(1/3) Correctly use recycled view and apply ViewHolder pattern

Mobile only. Release blocker.

Comment 22

[Lucas Rocha (:lucasr)]

Comment on attachment 610063 [details] [diff] [review]
(2/3) Factor out icon update into separate method

Mobile only. Release blocker.

Comment 23

[Lucas Rocha (:lucasr)]

Comment on attachment 610181 [details] [diff] [review]
Factor out checked state update into separate method

Mobile only. Release blocker.

Comment 24

[Martijn Wargers (dead)]

Regarding testcase http://people.mozilla.org/~mwargers/tests/forms/select/selectoptions_alot.html
This seems definitely fixed to me in yesterday's trunk build on the Samsung Galaxy Nexus.

Comment 25

[Alex Keybl [:akeybl]]

Comment on attachment 610062 [details] [diff] [review]
(1/3) Correctly use recycled view and apply ViewHolder pattern

[Triage Comment]
Mobile only & blocking Fennec 1.0. Approved for Aurora 13.

Comment 26

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/a6515bb070f4
https://hg.mozilla.org/releases/mozilla-aurora/rev/c2ababf0ea20
https://hg.mozilla.org/releases/mozilla-aurora/rev/c883b789280f