Closed
Bug 785246
Opened 12 years ago
Closed 12 years ago
BrowserToolbar can be rendered with low level window functions
Categories
(Firefox for Android Graveyard :: Theme and Visual Design, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 18
People
(Reporter: sriram, Unassigned)
References
Details
Attachments
(6 files, 1 obsolete file)
29.68 KB,
patch
|
mfinkle
:
review+
|
Details | Diff | Splinter Review |
70.65 KB,
patch
|
mfinkle
:
review+
|
Details | Diff | Splinter Review |
3.65 KB,
patch
|
mfinkle
:
review+
|
Details | Diff | Splinter Review |
39.50 KB,
patch
|
mfinkle
:
review+
|
Details | Diff | Splinter Review |
26.99 KB,
patch
|
mfinkle
:
review+
|
Details | Diff | Splinter Review |
9.89 KB,
patch
|
mfinkle
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•12 years ago
|
||
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
Reporter | ||
Comment 3•12 years ago
|
||
(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 4•12 years ago
|
||
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+
Reporter | ||
Comment 5•12 years ago
|
||
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)
Reporter | ||
Comment 6•12 years ago
|
||
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)
Reporter | ||
Comment 7•12 years ago
|
||
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)
Reporter | ||
Comment 8•12 years ago
|
||
This is for Menu Button. curveTowards:"none" is used for tablets.
Attachment #656570 -
Flags: review?(mark.finkle)
Reporter | ||
Comment 9•12 years ago
|
||
Attachment #656571 -
Flags: review?(mark.finkle)
Reporter | ||
Comment 10•12 years ago
|
||
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...)
Updated•12 years ago
|
Attachment #656565 -
Flags: review?(mark.finkle) → review+
Updated•12 years ago
|
Attachment #656567 -
Flags: review?(mark.finkle) → review+
Updated•12 years ago
|
Attachment #656568 -
Flags: review?(mark.finkle) → review+
Comment 11•12 years ago
|
||
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 12•12 years ago
|
||
Comment on attachment 656571 [details] [diff] [review] Patch (5/5): TabsPanel Button Actually, they are buttons!
Attachment #656571 -
Flags: review?(mark.finkle) → review+
Reporter | ||
Comment 13•12 years ago
|
||
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.
Reporter | ||
Comment 14•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/dbb57b1e7917 https://hg.mozilla.org/integration/mozilla-inbound/rev/987945f36cbc https://hg.mozilla.org/integration/mozilla-inbound/rev/d58c3359c422 https://hg.mozilla.org/integration/mozilla-inbound/rev/19b684e81c71 https://hg.mozilla.org/integration/mozilla-inbound/rev/98e88977a4b8
Reporter | ||
Comment 15•12 years ago
|
||
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 16•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/dbb57b1e7917 https://hg.mozilla.org/mozilla-central/rev/987945f36cbc https://hg.mozilla.org/mozilla-central/rev/d58c3359c422 https://hg.mozilla.org/mozilla-central/rev/19b684e81c71 https://hg.mozilla.org/mozilla-central/rev/98e88977a4b8
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18
Comment 17•12 years ago
|
||
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+
Reporter | ||
Comment 18•12 years ago
|
||
Pushed with rename: https://hg.mozilla.org/integration/mozilla-inbound/rev/d89a71a8f5ae
Updated•3 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
•