1091519 - Improve framerate of tab strip animations

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Lucas Rocha (:lucasr)]

The curve clipping technique used in each item in tab strip is greatly affecting the animations.

Comment 1

[Lucas Rocha (:lucasr)]

Attachment: New drawable framework for shaped views (r=mcomella)

Comment 2

[Lucas Rocha (:lucasr)]

Attachment: Re-implement tab strip shapes with ResizablePathDrawable (r=mcomella)

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8518230 [details] [diff] [review]
New drawable framework for shaped views (r=mcomella)

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

lgtm w/ nits.

::: mobile/android/base/widget/ResizablePathDrawable.java
@@ +13,5 @@
> +import android.graphics.drawable.ShapeDrawable;
> +import android.graphics.drawable.shapes.Shape;
> +
> +public class ResizablePathDrawable extends ShapeDrawable {
> +    private ColorStateList colors;

final?

nit: -> colorList/colorStateList

@@ +27,5 @@
> +        updateColor(getState());
> +    }
> +
> +    private boolean updateColor(int[] stateSet) {
> +        int newColor = colors.getColorForState(stateSet, 0);

We should use a color for the default, e.g. Color.WHITE.

0 also stands for Color.TRANSPARENT, which we probably don't want because it may make debugging a pain.

@@ +39,5 @@
> +    }
> +
> +    public Path getPath() {
> +        final ResizablePathShape shape = (ResizablePathShape) getShape();
> +        return shape.path;

Inner-access - should we be doing something here?

@@ +58,5 @@
> +    protected boolean onStateChange(int[] stateSet) {
> +        return updateColor(stateSet);
> +    }
> +
> +    public static class ResizablePathShape extends Shape {

-> NonScaledPathShape

Add a class comment (or in draw) that the original PathShape implementation scales; we choose not to (i.e. tell me your motivations for making this class and not extending PathShape directly but calling it PathShape anyway).

@@ +59,5 @@
> +        return updateColor(stateSet);
> +    }
> +
> +    public static class ResizablePathShape extends Shape {
> +        private Path path;

final?

@@ +61,5 @@
> +
> +    public static class ResizablePathShape extends Shape {
> +        private Path path;
> +        private float width;
> +        private float height;

Remove w/h.

@@ +77,5 @@
> +            }
> +
> +            canvas.save();
> +            canvas.drawPath(path, paint);
> +            canvas.restore();

Remove save/restore as discussed.

@@ +86,5 @@
> +        }
> +
> +        @Override
> +        public ResizablePathShape clone() throws CloneNotSupportedException {
> +            final ResizablePathShape shape = (ResizablePathShape) super.clone();

nit: -> clonedShape

@@ +88,5 @@
> +        @Override
> +        public ResizablePathShape clone() throws CloneNotSupportedException {
> +            final ResizablePathShape shape = (ResizablePathShape) super.clone();
> +
> +            if (path != null) {

Why would path == null?

Comment 4

[Lucas Rocha (:lucasr)]

(In reply to Michael Comella (:mcomella) from comment #3)
> Comment on attachment 8518230 [details] [diff] [review]
> New drawable framework for shaped views (r=mcomella)
> 
> Review of attachment 8518230 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> lgtm w/ nits.
> 
> ::: mobile/android/base/widget/ResizablePathDrawable.java
> @@ +13,5 @@
> > +import android.graphics.drawable.ShapeDrawable;
> > +import android.graphics.drawable.shapes.Shape;
> > +
> > +public class ResizablePathDrawable extends ShapeDrawable {
> > +    private ColorStateList colors;
> 
> final?

Done.

> nit: -> colorList/colorStateList

Done.

> @@ +27,5 @@
> > +        updateColor(getState());
> > +    }
> > +
> > +    private boolean updateColor(int[] stateSet) {
> > +        int newColor = colors.getColorForState(stateSet, 0);
> 
> We should use a color for the default, e.g. Color.WHITE.
> 
> 0 also stands for Color.TRANSPARENT, which we probably don't want because it
> may make debugging a pain.

Good point, done.

> @@ +39,5 @@
> > +    }
> > +
> > +    public Path getPath() {
> > +        final ResizablePathShape shape = (ResizablePathShape) getShape();
> > +        return shape.path;
> 
> Inner-access - should we be doing something here?

Not a big deal imo.

> @@ +58,5 @@
> > +    protected boolean onStateChange(int[] stateSet) {
> > +        return updateColor(stateSet);
> > +    }
> > +
> > +    public static class ResizablePathShape extends Shape {
> 
> -> NonScaledPathShape
> 
> Add a class comment (or in draw) that the original PathShape implementation
> scales; we choose not to (i.e. tell me your motivations for making this
> class and not extending PathShape directly but calling it PathShape anyway).

Done.

> @@ +59,5 @@
> > +        return updateColor(stateSet);
> > +    }
> > +
> > +    public static class ResizablePathShape extends Shape {
> > +        private Path path;
> 
> final?
> 
> @@ +61,5 @@
> > +
> > +    public static class ResizablePathShape extends Shape {
> > +        private Path path;
> > +        private float width;
> > +        private float height;
> 
> Remove w/h.

Done.
 
> @@ +77,5 @@
> > +            }
> > +
> > +            canvas.save();
> > +            canvas.drawPath(path, paint);
> > +            canvas.restore();
> 
> Remove save/restore as discussed.

Done.

> @@ +86,5 @@
> > +        }
> > +
> > +        @Override
> > +        public ResizablePathShape clone() throws CloneNotSupportedException {
> > +            final ResizablePathShape shape = (ResizablePathShape) super.clone();
> 
> nit: -> clonedShape

Done.

> @@ +88,5 @@
> > +        @Override
> > +        public ResizablePathShape clone() throws CloneNotSupportedException {
> > +            final ResizablePathShape shape = (ResizablePathShape) super.clone();
> > +
> > +            if (path != null) {
> 
> Why would path == null?

Just me being paranoid. Removed null check.

Comment 5

[Lucas Rocha (:lucasr)]

https://hg.mozilla.org/integration/fx-team/rev/510682d510e9
https://hg.mozilla.org/integration/fx-team/rev/fbd3fe5b8d9b

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8518231 [details] [diff] [review]
Re-implement tab strip shapes with ResizablePathDrawable (r=mcomella)

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

Don't forget to re-open the fading edge bug, if you have not already.

::: mobile/android/base/newtablet/res/layout-large-v11/tab_strip.xml
@@ -10,5 @@
>          android:layout_width="match_parent"
>          android:layout_height="match_parent"
>          android:layout_weight="1"
> -        android:requiresFadingEdge="horizontal"
> -        android:fadingEdgeLength="10dp"

This should probably be in a separate part, but I won't stop you.

::: mobile/android/base/tabs/TabCurve.java
@@ +39,5 @@
>  
>      private TabCurve() {
>      }
>  
> +    public static float getWidthForHeight(float height) {

Why did this get changed to float? (or better yet, why was it int to begin with?)

Comment 7

[Lucas Rocha (:lucasr)]

(In reply to Michael Comella (:mcomella) from comment #6)
> Comment on attachment 8518231 [details] [diff] [review]
> Re-implement tab strip shapes with ResizablePathDrawable (r=mcomella)
> 
> Review of attachment 8518231 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Don't forget to re-open the fading edge bug, if you have not already.

Done.
 
> ::: mobile/android/base/newtablet/res/layout-large-v11/tab_strip.xml
> @@ -10,5 @@
> >          android:layout_width="match_parent"
> >          android:layout_height="match_parent"
> >          android:layout_weight="1"
> > -        android:requiresFadingEdge="horizontal"
> > -        android:fadingEdgeLength="10dp"
> 
> This should probably be in a separate part, but I won't stop you.

True.

> ::: mobile/android/base/tabs/TabCurve.java
> @@ +39,5 @@
> >  
> >      private TabCurve() {
> >      }
> >  
> > +    public static float getWidthForHeight(float height) {
> 
> Why did this get changed to float? (or better yet, why was it int to begin
> with?)

Because the drawable APIs are all float-based. Just to avoid annoying type castings.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/510682d510e9
https://hg.mozilla.org/mozilla-central/rev/fbd3fe5b8d9b