1056012 - Split ShapedButton into PhoneTabsButton and TabletTabsButton

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Lucas Rocha (:lucasr)]

It doesn't make sense to keep them together in ShapedButton anymore as the curve shapes are different now.

Comment 1

[Lucas Rocha (:lucasr)]

Attachment: Split ShapedButton into PhoneTabsButton and TabletTabsButton (r=mcomella)

Comment 2

[Lucas Rocha (:lucasr)]

Comment on attachment 8475865 [details] [diff] [review]
Split ShapedButton into PhoneTabsButton and TabletTabsButton (r=mcomella)

I have to say, I'm really not a fan of how the whole ShapedButton/CanvasDelegate is structured. This patch simply separates the curve shape implementation between tablet and phones.

Comment 3

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

Comment on attachment 8475865 [details] [diff] [review]
Split ShapedButton into PhoneTabsButton and TabletTabsButton (r=mcomella)

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

::: mobile/android/base/toolbar/BackButton.java
@@ -16,5 @@
>  import android.graphics.drawable.StateListDrawable;
>  import android.util.AttributeSet;
>  
>  public class BackButton extends ShapedButton { 
> -    private Path mPath;

Why did you delete this member? It is used in onSizeChanged.

Comment 4

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

Comment on attachment 8475865 [details] [diff] [review]
Split ShapedButton into PhoneTabsButton and TabletTabsButton (r=mcomella)

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

Better yet, r+, provided you remove the mPath removal in BackButton.

Comment 5

[Lucas Rocha (:lucasr)]

(In reply to Michael Comella (:mcomella) from comment #4)
> Comment on attachment 8475865 [details] [diff] [review]
> Split ShapedButton into PhoneTabsButton and TabletTabsButton (r=mcomella)
> 
> Review of attachment 8475865 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Better yet, r+, provided you remove the mPath removal in BackButton.

Classic example that doing *just a bit* more than the intended change causes confusion :-) I made mPath protected in ShapedButton to avoid redundant Path instances in subclasses. mPath is not needed anymore in BackButton. I should have changed BackButton in a separate patch to make this clearer.

Comment 6

[Lucas Rocha (:lucasr)]

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

Comment 7

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

https://hg.mozilla.org/mozilla-central/rev/da30c476bf50