933459 - Reduce the drawables created by FaviconView

Status: RESOLVED FIXED

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

Description

[Sriram Ramasubramanian [:sriram]]

By getting a drawable and applying a color filter, there are so many of them recreated every time a user scrolls. This doesn't feel any good. It's better to draw everything to the Canvas directly.

Comment 1

[Sriram Ramasubramanian [:sriram]]

============= Spam ahead =============

I/Sriram  ( 9439): drawable: android.graphics.drawable.GradientDrawable@42a69188
I/Sriram  ( 9439): drawable: android.graphics.drawable.GradientDrawable@4288f830
I/Sriram  ( 9439): drawable: android.graphics.drawable.GradientDrawable@4299a298
I/Sriram  ( 9439): drawable: android.graphics.drawable.GradientDrawable@4299e268
I/Sriram  ( 9439): drawable: android.graphics.drawable.GradientDrawable@429a4bd8
I/Sriram  ( 9439): drawable: android.graphics.drawable.GradientDrawable@429a8c50
I/Sriram  ( 9439): drawable: android.graphics.drawable.GradientDrawable@42a4a540
I/Sriram  ( 9439): drawable: android.graphics.drawable.GradientDrawable@42a4ac20
I/Sriram  ( 9439): drawable: android.graphics.drawable.GradientDrawable@42a64de0
I/Sriram  ( 9439): drawable: android.graphics.drawable.GradientDrawable@42d4c228
I/Sriram  ( 9439): drawable: android.graphics.drawable.GradientDrawable@42ab92e8
I/Sriram  ( 9439): drawable: android.graphics.drawable.GradientDrawable@42a99580
I/Sriram  ( 9439): drawable: android.graphics.drawable.GradientDrawable@42d56478
I/Sriram  ( 9439): drawable: android.graphics.drawable.GradientDrawable@42a66108
I/Sriram  ( 9439): drawable: android.graphics.drawable.GradientDrawable@42a59438
I/Sriram  ( 9439): drawable: android.graphics.drawable.GradientDrawable@42a74cc8
I/Sriram  ( 9439): drawable: android.graphics.drawable.GradientDrawable@42d5d280
I/Sriram  ( 9439): drawable: android.graphics.drawable.GradientDrawable@42a6e598
I/Sriram  ( 9439): drawable: android.graphics.drawable.GradientDrawable@42ac85d8
I/Sriram  ( 9439): drawable: android.graphics.drawable.GradientDrawable@42b71510
I/Sriram  ( 9439): drawable: android.graphics.drawable.GradientDrawable@42b72bc8
I/Sriram  ( 9439): drawable: android.graphics.drawable.GradientDrawable@42b7d450
I/Sriram  ( 9439): drawable: android.graphics.drawable.GradientDrawable@42b7dfe8
I/Sriram  ( 9439): drawable: android.graphics.drawable.GradientDrawable@42b7eb60
I/Sriram  ( 9439): drawable: android.graphics.drawable.GradientDrawable@42d7dda0
I/Sriram  ( 9439): drawable: android.graphics.drawable.GradientDrawable@42d7eb10
I/Sriram  ( 9439): drawable: android.graphics.drawable.GradientDrawable@42d7f858
I/Sriram  ( 9439): drawable: android.graphics.drawable.GradientDrawable@42d804a0
I/Sriram  ( 9439): drawable: android.graphics.drawable.GradientDrawable@42d81088
I/Sriram  ( 9439): drawable: android.graphics.drawable.GradientDrawable@42d81eb8
I/Sriram  ( 9439): drawable: android.graphics.drawable.GradientDrawable@42d82ca8
I/Sriram  ( 9439): drawable: android.graphics.drawable.GradientDrawable@42d83a88
I/Sriram  ( 9439): drawable: android.graphics.drawable.GradientDrawable@42d84780
I/Sriram  ( 9439): drawable: android.graphics.drawable.GradientDrawable@42d88468
I/Sriram  ( 9439): drawable: android.graphics.drawable.GradientDrawable@42d89000
I/Sriram  ( 9439): drawable: android.graphics.drawable.GradientDrawable@42d89b78
I/Sriram  ( 9439): drawable: android.graphics.drawable.GradientDrawable@42d8c660
I/Sriram  ( 9439): drawable: android.graphics.drawable.GradientDrawable@42d8d3d0
I/Sriram  ( 9439): drawable: android.graphics.drawable.GradientDrawable@42d8e118
I/Sriram  ( 9439): drawable: android.graphics.drawable.GradientDrawable@42d8ecc8
I/Sriram  ( 9439): drawable: android.graphics.drawable.GradientDrawable@42d93528
I/Sriram  ( 9439): drawable: android.graphics.drawable.GradientDrawable@42d948c0
I/Sriram  ( 9439): drawable: android.graphics.drawable.GradientDrawable@42d956f0
I/Sriram  ( 9439): drawable: android.graphics.drawable.GradientDrawable@42d964e0
I/Sriram  ( 9439): drawable: android.graphics.drawable.GradientDrawable@42d972c0
I/Sriram  ( 9439): drawable: android.graphics.drawable.GradientDrawable@42d97fb8

============ End of Spam =============

Comment 2

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

Android doesn't allow setting a different color for the stroke. Hence creating 2 paint objects, and 2 rectangles to track it. And these are static and shared between multiple Favicons. So all good (as long as FaviconViews are of same size). No drawables are involved and we draw directly to the canvas.

Comment 3

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

Comment on attachment 825553 [details] [diff] [review]
Patch

> so all good (as long as FaviconViews are of same size)

And what happens if this is not the case? Can we add a Log.w to at least tell us if we run into a situation where a FaviconView is a different size for some reason?

Comment 4

[Sriram Ramasubramanian [:sriram]]

I am not sure if that is going to help. Say there are two favicon views, one of size 24x24dp and the other 36x36dp. The rectangles would change during onSizeChanged() (I should kill that!). Now depending on who is drawn first, the size may or may not match it.

But currently all FaviconViews are of same size, and we won't run into this problem.

Comment 5

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch 2

This makes the RectFs to be class members. That way, the paint is shared, but the rects are new for each instance. (That's just 8 float values).

Comment 6

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

Comment on attachment 826084 [details] [diff] [review]
Patch 2

Also looks fine

Comment 7

[Sriram Ramasubramanian [:sriram]]

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

Comment 8

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

https://hg.mozilla.org/mozilla-central/rev/17cc01937229

Comment 9

[:Margaret Leibovic]

Comment on attachment 826084 [details] [diff] [review]
Patch 2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): this patch improves the favicon-rewrite related bug reported in bug 926646 (that white backgrounds appear behind small favicons)
User impact if declined: broken-looking white backgrounds appear behind small favicons
Testing completed (on m-c, etc.): landed on m-c 11/4, no regressions reported
Risk to taking this patch (and alternatives if risky): this has baked on Nightly for a while, and the changes are limited to the views that display favicons
String or IDL/UUID changes made by this patch: none

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e072083c4a0d