744537 - Use CustomViews instead of ViewHolders in AwesomeBarTabs

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: General, defect)

Description

[Sriram Ramasubramanian [:sriram]]

Using ViewHolders increases the performance of lists, as they decrease the number of "findViewById()" made to set a row's value in a list.

In our case, the AwesomeBarTabs uses the same row layout for 3 different lists. And in each Adapter, the row is inflated and findViewById() is called to store in a ViewHolder. Instead, this can be made into a custom view -- say AwesomeBarRow, which takes care of the findViewById() during inflation. The performance will remain the same, and the view will be logically separated out. This makes reusability of code with all the 3 different list adapters.

Comment 1

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

I cleanup my patch from last week and ensured that there is no performance difference between both. I feel this is a better approach.

Also, we can implement RecyleListeners with list adapters, to clear of data held by the row -- say the drawable favicon that is being displayed. This makes sure less memory is used to show the lists (and the recycled views basically have nothing but the inflated views).

Comment 2

[Lucas Rocha (:lucasr)]

Comment on attachment 614110 [details] [diff] [review]
Patch

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

I think you're going a bit overboard with the code splitting there. Viewholder is a very simple class (three public properties, that's all). The amount and nature of reused code in your patch is not worth the separation, really.

If you want to make AwesomeBarTabs a bit less chunky, I'd suggest splitting the code to handle each tab into separate classes. That's the type of separation that would actually make this code easier to maintain.

Comment 3

[Sriram Ramasubramanian [:sriram]]

I guess I'm talking about clear abstraction of code into cleaner classes. I don't understand what is overboard here! :)

In your ViewHolder approach:
The layout inflation happens somewhere.
The properties are held by ViewHolder.
And the code is all split in different places.

In my approach:
The layout inflation and the properties are held by the custom class -- as it should be.
The code looks cleaner.

If you want to have "public" properties in the custom classes, I can do it (though I don't like it. As, for me, row.setTitle("xyz") is better than row.title.setText("xyz"). )

Comment 4

[Sriram Ramasubramanian [:sriram]]

This approach isn't needed.