Closed
Bug 1091519
Opened 10 years ago
Closed 10 years ago
Improve framerate of tab strip animations
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 36
People
(Reporter: lucasr, Assigned: lucasr)
References
Details
Attachments
(2 files)
(deleted),
patch
|
mcomella
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mcomella
:
review+
|
Details | Diff | Splinter Review |
The curve clipping technique used in each item in tab strip is greatly affecting the animations.
Assignee | ||
Updated•10 years ago
|
Summary: Improve framework of tab strip animations → Improve framerate of tab strip animations
Assignee | ||
Comment 1•10 years ago
|
||
Assignee | ||
Comment 2•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8518231 -
Flags: review?(michael.l.comella)
Assignee | ||
Updated•10 years ago
|
Attachment #8518230 -
Flags: review?(michael.l.comella)
Comment 3•10 years ago
|
||
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?
Attachment #8518230 -
Flags: review?(michael.l.comella) → review+
Assignee | ||
Comment 4•10 years ago
|
||
(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.
Assignee | ||
Comment 5•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/510682d510e9 https://hg.mozilla.org/integration/fx-team/rev/fbd3fe5b8d9b
Comment 6•10 years ago
|
||
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?)
Attachment #8518231 -
Flags: review?(michael.l.comella) → review+
Updated•10 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 7•10 years ago
|
||
(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•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/510682d510e9 https://hg.mozilla.org/mozilla-central/rev/fbd3fe5b8d9b
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•