Closed Bug 785246 Opened 7 years ago Closed 7 years ago

BrowserToolbar can be rendered with low level window functions

Categories

(Firefox for Android :: Theme and Visual Design, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 18

People

(Reporter: sriram, Unassigned)

References

Details

Attachments

(6 files, 1 obsolete file)

The curves can be rendered using low level bezier curve functions. This will allow the entire background to be one single (repeating) image -- which is a pre-requisite for Personas.
Attached patch Patch (obsolete) — Splinter Review
This redraws the curve on the Canvas. There won't be any performance hit, as this is how android draws every view, and we dont do complex alpha compositing there.

Couple of comments:
1. mPath is created during onMeasure(), as we know the size there. We needn't create a path everytime during onDraw. Our rotation re-inflates the view (even if it doesn't, a size change triggers onMeasure()).

2. Paint is just for anti-aliasing. Ideally clipping can be done using clipPath(). However, clipping (a) doesn't have anti-aliasing, (b) cannot be used with hardware acceleration.

3. The layer has to be saved and the drawPath() has to be done offscreen, as doing directly on surface will show black portions.

I have a thought of moving the magic numbers to R.fractions.something. I am not sure if we want to move it there, are have it here. Anyways, it's a one time thing (atleast until we do it for tabs and other curves).
Attachment #654820 - Flags: review?(mark.finkle)
Comment on attachment 654820 [details] [diff] [review]
Patch

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

::: mobile/android/base/BrowserToolbarBackground.java
@@ +13,5 @@
> +import android.graphics.RectF;
> +import android.util.AttributeSet;
> +import android.widget.LinearLayout;
> +
> +public class BrowserToolbarBackground extends LinearLayout { 

it says there's a space here

@@ +33,5 @@
> +        super.onMeasure(widthMeasureSpec, heightMeasureSpec);
> +
> +        int width = getMeasuredWidth();
> +        int height = getMeasuredHeight();
> +        float curveWidth = (float) (height * 54/48);

casting to float after the arithmetic... that's to make sure things are snapped to pixel boundaries, right?

@@ +36,5 @@
> +        int height = getMeasuredHeight();
> +        float curveWidth = (float) (height * 54/48);
> +
> +        // Path specifies the portion to be clipped.
> +        mPath = new Path();

I think it would be cool if this just used rewind() instead of allocating another object

@@ +39,5 @@
> +        // Path specifies the portion to be clipped.
> +        mPath = new Path();
> +        mPath.moveTo(width, height);
> +        mPath.cubicTo((width - (float) (curveWidth * 0.75)), height,
> +                      (width - (float) (curveWidth * 0.25)), 0,

using 0.75f and 0.25f would be more compact
(In reply to microrffr from comment #2)
> Comment on attachment 654820 [details] [diff] [review]
> Patch
> 
> Review of attachment 654820 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +36,5 @@
> > +        int height = getMeasuredHeight();
> > +        float curveWidth = (float) (height * 54/48);
> > +
> > +        // Path specifies the portion to be clipped.
> > +        mPath = new Path();
> 
> I think it would be cool if this just used rewind() instead of allocating
> another object
> 
onMeasure() is called only when there is a size change. I get the "reset()" part of Path. But we wouldn't ever get onMeasure() again. We are re-inflation on rotation. I can add logs and see. If I get more onMeasure(), I would be happy to change it to reset().
Comment on attachment 654820 [details] [diff] [review]
Patch

>diff --git a/mobile/android/base/BrowserToolbarBackground.java b/mobile/android/base/BrowserToolbarBackground.java

>+    public void onMeasure(int widthMeasureSpec, int heightMeasureSpec) {

>+        float curveWidth = (float) (height * 54/48);

I worry about integer truncation whenever I see hardcoded int divsion. Can we just use 1.125f ?

>+    public void draw(Canvas canvas) {

>+                                  Canvas.CLIP_TO_LAYER_SAVE_FLAG);
>+
>+
>+        // Do a default draw.

nit: Remove the extra blank line in there
Attachment #654820 - Flags: review?(mark.finkle) → review+
This is the base patch. Working on other curvy curvy things, I figured out that the Canvas painting logic is the same. Hence I moved it to a class called CanvasDelegate, which will do the painting for us.

However, CanvasDelegate doesn't know how to draw the default background. It cannot call "super.draw(canvas)". Hence, the "DrawManager", who delegates the draw() to CanvasDelegate, does a defaultDraw(). This is the base logic for all the following patches.

** Killed 9 images **
Attachment #654820 - Attachment is obsolete: true
Attachment #656565 - Flags: review?(mark.finkle)
The hardest patch in the lot.

TabsButton supports two attributes: curveTowards and cropped.
curveTowards specifies the direction of the curve: right for phones (default), left for tablets, and a none (for reusing the attribute with menus).
cropped specifies if a "black" portion should be drawn or not.

On phones, the cropped button will need an additional "black" portion on the right corner (or tabs with curveTowards:"right"). Also, this needn't be drawn in expanded state -- as it's transparent.

On tablets, (aaaaahhhhh...), the expanded state should have only one curve. That's done in onMeasure(), based on the "getLevel()" of the drawable. I couldn't find a better way to make things clear, as our TabsButton is way too complicated. (However, there is no performance regressions).
Attachment #656567 - Flags: review?(mark.finkle)
Cropped button needed a better curve. The Patch (2/5), doesn't do it. This patch adds a better right curve (for right curved tabs). This is not perfect yet. This is more of a placeholder until Ian comes back.
Attachment #656568 - Flags: review?(mark.finkle)
This is for Menu Button.
curveTowards:"none" is used for tablets.
Attachment #656570 - Flags: review?(mark.finkle)
Attachment #656571 - Flags: review?(mark.finkle)
The number of images killed:
Patch (1/5): 9 images
Patch (2/5): 48 images
Patch (4/5): 18 images
Patch (5/5): 18 images.

Additionally, the original mockups had "curved" add-tab buttons. I had to kill them for the sake of lesser images. We can add it now easily, so that, the sliding looks better (instead of black to filled portion).

Ian's images had a shadow. (Errrrrr...) We can add shadows to Canvas. However, he was moving towards a non-shadow-y version. We can wait to hear from him on that.

(Pleasee... Ian.. Pleaseee...)
Attachment #656565 - Flags: review?(mark.finkle) → review+
Attachment #656567 - Flags: review?(mark.finkle) → review+
Attachment #656568 - Flags: review?(mark.finkle) → review+
Comment on attachment 656570 [details] [diff] [review]
Patch (4/5): Menu

I wonder if we should rename these files. They are not really Buttons. More like something drawable.
Attachment #656570 - Flags: review?(mark.finkle) → review+
Comment on attachment 656571 [details] [diff] [review]
Patch (5/5): TabsPanel Button

Actually, they are buttons!
Attachment #656571 - Flags: review?(mark.finkle) → review+
They are still ImageButtons. But then, I was also thinking of have a super class of CurvedButton, and then subclassing them from it. I can take it as a followup.
This moves the common code to a super-class (ummm..) named CurvedButton. The classes extending it just needs to update the path and whether the path has to be masked or clipped.
Attachment #657414 - Flags: review?(mark.finkle)
Comment on attachment 657414 [details] [diff] [review]
Patch (6/6): Refactor (this soon?)

Just requesting a name change: CurvedButton -> ShapedButton
Attachment #657414 - Flags: review?(mark.finkle) → review+
Depends on: 787627
You need to log in before you can comment on or make changes to this bug.