Closed Bug 933459 Opened 6 years ago Closed 6 years ago

Reduce the drawables created by FaviconView

Categories

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

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 28
Tracking Status
firefox27 --- fixed
firefox28 --- fixed

People

(Reporter: sriram, Assigned: sriram)

Details

Attachments

(1 file, 1 obsolete file)

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.
============= 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 =============
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #825553 - Flags: review?(mark.finkle)
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?
Attachment #825553 - Flags: review?(mark.finkle) → review+
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.
Attached patch Patch 2Splinter Review
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).
Attachment #826084 - Flags: review?(mark.finkle)
Comment on attachment 826084 [details] [diff] [review]
Patch 2

Also looks fine
Attachment #826084 - Flags: review?(mark.finkle) → review+
https://hg.mozilla.org/mozilla-central/rev/17cc01937229
Assignee: nobody → sriram
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 28
Attachment #825553 - Attachment is obsolete: true
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
Attachment #826084 - Flags: approval-mozilla-aurora?
Attachment #826084 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.