900240 - FadedTextView creates objects in onDraw()

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: Theme and Visual Design, defect)

Description

[Sriram Ramasubramanian [:sriram]]

Creating objects in onDraw is a performance problem. It's better to do it somewhere else, where the Gradient needn't be changed.

Comment 1

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

It's good to avoid object creation in onDraw() call. Now with the profile GPU option it is clearly evident that onDraw() is doing more work.

This offsets that by moving it to onSizeChanged(). Though it is recommended not to create objects in onXYZ() methods, we cannot avoid in this case. The onSizeChanged() is called only once on creation of the view. And the maximum number of views created are no. of visible row items + 1. So, the number of times shader is created is about 8-10 times on a phone.

The draw works as usual. This is a nice improvement.
The two disadvantages:
1. This FadedTextView cannot use a state-list drawable anymore. So, if there is a state change, we need to explicitly change the shader.
2. Even though the view might not need a shader applied as the text is short, this method would still apply. (But display lists will take care of it :P).

Comment 2

[Sriram Ramasubramanian [:sriram]]

I got confused about the lines it showed. So, the colors are as follows:
1. blue is for draw()
2. red is for processing display lists
3. yellow is for swapping buffers.

So, there is not really great improvement in the blue bar. We may/may not need this patch :P
I leave it to you to see if it increases performance or not. I'm still confused on the fact that the shader is applied for all TextViews. This might not be a problem for 4.0+. But on older phones with software model, may be we could avoid.

Comment 3

[Lucas Rocha (:lucasr)]

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

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

Avoiding creating new objects in onDraw is definitely a good thing to do. But I'm not sure I understand why we need to show the gradient even when the text is shorter than the view width. Giving r- to get some answers first.

::: mobile/android/base/home/FadedTextView.java
@@ +45,2 @@
>  
> +        int color = getCurrentTextColor();

final

@@ +45,3 @@
>  
> +        int color = getCurrentTextColor();
> +        float stop = ((float) (width - mFadeWidth) / (float) width);

final

@@ +45,4 @@
>  
> +        int color = getCurrentTextColor();
> +        float stop = ((float) (width - mFadeWidth) / (float) width);
> +        LinearGradient gradient = new LinearGradient(0, 0, width, 0,

final

@@ -49,4 @@
>  
> -        // Layout doesn't return a proper width for getWidth().
> -        // Instead check the width of the first line, as we've restricted to just one line.
> -        if (getLayout().getLineWidth(0) > width) {

Why not doing this check anymore? We don't need to show the gradient if the text doesn't extend beyond the view boundaries. Or am I missing something?

::: mobile/android/base/resources/layout/two_line_page_row.xml
@@ +19,5 @@
>  
>          <org.mozilla.gecko.home.FadedTextView
>                  android:id="@+id/title"
>                  style="@style/Widget.TwoLinePageRow.Title"
> +                android:layout_width="fill_parent"

This change shouldn't be necessary if the gradient is properly shown/hidden depending on the text/view boundaries.

Comment 4

[Sriram Ramasubramanian [:sriram]]

> @@ -49,4 @@
> >  
> > -        // Layout doesn't return a proper width for getWidth().
> > -        // Instead check the width of the first line, as we've restricted to just one line.
> > -        if (getLayout().getLineWidth(0) > width) {
> 
> Why not doing this check anymore? We don't need to show the gradient if the
> text doesn't extend beyond the view boundaries. Or am I missing something?

We've moved the gradient calculation to onSizeChanged(). To be optimal in not doing layout pass again and again, and reuse the same view, I used the "fill_parent" width below. In that case, all views extend until right end. Hence we don't need this check anymore.

> 
> ::: mobile/android/base/resources/layout/two_line_page_row.xml
> @@ +19,5 @@
> >  
> >          <org.mozilla.gecko.home.FadedTextView
> >                  android:id="@+id/title"
> >                  style="@style/Widget.TwoLinePageRow.Title"
> > +                android:layout_width="fill_parent"
> 
> This change shouldn't be necessary if the gradient is properly shown/hidden
> depending on the text/view boundaries.


I think we might need a state list for FadedTextView, if we are going to support personas. I would probably wait until we make that decision before proceeding on this patch.

Comment 5

[Lucas Rocha (:lucasr)]

(In reply to Sriram Ramasubramanian [:sriram] from comment #4)
> > @@ -49,4 @@
> > >  
> > > -        // Layout doesn't return a proper width for getWidth().
> > > -        // Instead check the width of the first line, as we've restricted to just one line.
> > > -        if (getLayout().getLineWidth(0) > width) {
> > 
> > Why not doing this check anymore? We don't need to show the gradient if the
> > text doesn't extend beyond the view boundaries. Or am I missing something?
> 
> We've moved the gradient calculation to onSizeChanged(). To be optimal in
> not doing layout pass again and again, and reuse the same view, I used the
> "fill_parent" width below. In that case, all views extend until right end.
> Hence we don't need this check anymore.

Hmmm, assuming a view will always be used with 'fill_parent' is probably not a good idea. Very prone to confusion.

Comment 6

[Sriram Ramasubramanian [:sriram]]

Why so? Don't we use "fill_parent" in other cases? Even if we wanted to show just an ellipsis, the width would have been "fill_parent", right?

Comment 7

[Lucas Rocha (:lucasr)]

(In reply to Sriram Ramasubramanian [:sriram] from comment #6)
> Why so? Don't we use "fill_parent" in other cases? Even if we wanted to show
> just an ellipsis, the width would have been "fill_parent", right?

If you want FadedTextView to be a general-purpose view, it should not assume anything about the context in which it will be used. If you really want to keep the "this should be used with fill_parent" invariant for this view, you should at least throw an exception if someone tries to use a different layout param.

Comment 8

[Sriram Ramasubramanian [:sriram]]

Barely any improvement in actual drawing. It's better to leave it the way it is, as that would help in using multiple states with the FadedTextView if needed.