Closed Bug 1056012 Opened 6 years ago Closed 6 years ago

Split ShapedButton into PhoneTabsButton and TabletTabsButton

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 34

People

(Reporter: lucasr, Assigned: lucasr)

References

Details

Attachments

(1 file)

It doesn't make sense to keep them together in ShapedButton anymore as the curve shapes are different now.
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.
Attachment #8475865 - Flags: review?(michael.l.comella)
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.
Attachment #8475865 - Flags: review?(michael.l.comella) → review-
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.
Attachment #8475865 - Flags: review- → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/da30c476bf50
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
You need to log in before you can comment on or make changes to this bug.