1055576 - Factor out code to draw tab curves

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Lucas Rocha (:lucasr)]

So that we can reuse it from different places in our code base.

Comment 1

[Lucas Rocha (:lucasr)]

Attachment: Factor out code to draw tab curves (r=mcomella)

Comment 2

[Lucas Rocha (:lucasr)]

Comment on attachment 8475213 [details] [diff] [review]
Factor out code to draw tab curves (r=mcomella)

I'll land this directly in m-c.

Comment 3

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

Comment on attachment 8475213 [details] [diff] [review]
Factor out code to draw tab curves (r=mcomella)

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

::: mobile/android/base/tabs/TabCurve.java
@@ +40,5 @@
> +                     from + width * 0.723f * dir.value, height * 0.961f,
> +                     from + width * dir.value, height);
> +    }
> +
> +    public static void drawFromBottom(Path path, int from, int height, Direction dir) {

This is unused - when might we want to draw from the bottom?

I can't double-check these constants because I don't know where they come from, but I'm surprised they're not shared with drawFromTop.

::: mobile/android/base/toolbar/ShapedButton.java
@@ -72,4 @@
>              mPath.lineTo(width, height);
>              mPath.lineTo(width, 0);
>              mPath.lineTo(0, 0);
>          } else if (mSide == CurveTowards.LEFT) {

You should also use the TabCurve method here in CurveTowards.LEFT, right?

Comment 4

[Lucas Rocha (:lucasr)]

(In reply to Michael Comella (:mcomella) from comment #3)
> Comment on attachment 8475213 [details] [diff] [review]
> Factor out code to draw tab curves (r=mcomella)
> 
> Review of attachment 8475213 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/tabs/TabCurve.java
> @@ +40,5 @@
> > +                     from + width * 0.723f * dir.value, height * 0.961f,
> > +                     from + width * dir.value, height);
> > +    }
> > +
> > +    public static void drawFromBottom(Path path, int from, int height, Direction dir) {
> 
> This is unused - when might we want to draw from the bottom?

This is used in the tab strip.

> I can't double-check these constants because I don't know where they come
> from, but I'm surprised they're not shared with drawFromTop.

Good point, let me see if we can share some values between both methods.

> ::: mobile/android/base/toolbar/ShapedButton.java
> @@ -72,4 @@
> >              mPath.lineTo(width, height);
> >              mPath.lineTo(width, 0);
> >              mPath.lineTo(0, 0);
> >          } else if (mSide == CurveTowards.LEFT) {
> 
> You should also use the TabCurve method here in CurveTowards.LEFT, right?

Nope. This is used by the current tablet UI and should remain unchanged. I'll add a comment about this for now. Filed bug 1056012 to split the tablet and phone implementations more clearly.

ps: there's a lot of historical cruft/nonsense in the ShapedButton code and subclasses. Definitely needs some cleanups but it's not urgent.

Comment 5

[Lucas Rocha (:lucasr)]

Attachment: Factor out code to draw tab curves (r=mcomella)

Comment 6

[Lucas Rocha (:lucasr)]

Comment on attachment 8475856 [details] [diff] [review]
Factor out code to draw tab curves (r=mcomella)

Now sharing the multipliers between drawFromTop and drawFromBottom.

Comment 7

[Lucas Rocha (:lucasr)]

https://hg.mozilla.org/integration/fx-team/rev/6140dde847b5

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6140dde847b5