934658 - Favicon in url bar causing non-rectangular clip operation

Status: RESOLVED FIXED

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

Description

[Sriram Ramasubramanian [:sriram]]

The ImageView, showing the favicon, is rotating about itself. This rotation causes non-rectangular clipping. Non-rectangular clipping can cause performance problems.

Comment 1

[Sriram Ramasubramanian [:sriram]]

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.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 3

[Sriram Ramasubramanian [:sriram]]

Attachment: 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.

Comment 4

[Sriram Ramasubramanian [:sriram]]

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

Comment 5

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

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

Comment 6

[Lucas Rocha (:lucasr)]

(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.

Comment 7

[Sriram Ramasubramanian [: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.

Comment 8

[Richard Newman [:rnewman]]

Lucas, mfinkle: should we undo this change now that we don't use the spinning throbber?

Comment 9

[Lucas Rocha (:lucasr)]

(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.