Closed Bug 934658 Opened 6 years ago Closed 6 years ago

Favicon in url bar causing non-rectangular clip operation

Categories

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

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: sriram, Assigned: sriram)

Details

Attachments

(1 file)

The ImageView, showing the favicon, is rotating about itself. This rotation causes non-rectangular clipping. Non-rectangular clipping can cause performance problems.
This can be avoided by using a hardware layer on the view. However that is not available before honeycomb. Probably we should rotate the drawable.
If we can use a simple & better, hardware layer, fix for Honeycomb and higher, let's do it. If we need a different (not better) fix for gingerbread, so be it. Gingerbread is 15% of our users right now. No sense in doing a harder fix for all platform versions.
Attached patch PatchSplinter Review
This makes it back the View with a hardware layer. I don't want to do it in XML as its common for all versions. And I don't feel like creating styles and hiding it there. This feels better. And no more non-rect clipping now.

Regarding performance, things are below 16ms. But I don't know how much gecko loading is affecting it. May be other metrics and tests we have in infrastructure can help us find it.
Attachment #827086 - Flags: review?(mark.finkle)
Attachment #827086 - Flags: review?(mark.finkle) → review+
https://hg.mozilla.org/mozilla-central/rev/e8aca8cb2756
Assignee: nobody → sriram
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 28
(In reply to Sriram Ramasubramanian [:sriram] from comment #3)
> Created attachment 827086 [details] [diff] [review]
> Patch
> 
> This makes it back the View with a hardware layer. I don't want to do it in
> XML as its common for all versions. And I don't feel like creating styles
> and hiding it there. This feels better. And no more non-rect clipping now.
> 
> Regarding performance, things are below 16ms. But I don't know how much
> gecko loading is affecting it. May be other metrics and tests we have in
> infrastructure can help us find it.

Drive-by: ideally, we shouldn't be adding permanent hw layers in our UI as they eat GPU memory to cache the GL texture. You should only set the hw layer while animating and remove it once the throbber stops. 

The favicon is a small view but still... file a follow-up to handle that.
Flags: needinfo?(sriram)
I actually had a patch to do that. But I didn't feel a big difference with it. Also, it felt like having issues with changing the favicon and throbber. So went ahead with hardware backing always. A small texture wont be a problem I guess.
Flags: needinfo?(sriram)
Lucas, mfinkle: should we undo this change now that we don't use the spinning throbber?
Flags: needinfo?(lucasr.at.mozilla)
(In reply to Richard Newman [:rnewman] from comment #8)
> Lucas, mfinkle: should we undo this change now that we don't use the
> spinning throbber?

Nice catch. This should have been removed as part of bug 917896. I even pointed this out in my review. It turns out bug 968180 will back this out.
Flags: needinfo?(lucasr.at.mozilla)
You need to log in before you can comment on or make changes to this bug.