Closed Bug 888032 Opened 12 years ago Closed 12 years ago

TopBookmarkItemView's favicon should be restricted to 32dp

Categories

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

ARM
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 26

People

(Reporter: sriram, Assigned: sriram)

References

Details

(Whiteboard: fixed-fig)

Attachments

(1 file)

If there is no valid thumbnail available, we show a favicon. There is no scaling involved, and this can be really huge (256x256). Instead show a 32dp favicon in such cases.
Attached patch PatchSplinter Review
Attachment #768599 - Flags: review?(wjohnston)
Blocks: 882365
Comment on attachment 768599 [details] [diff] [review] Patch Review of attachment 768599 [details] [diff] [review]: ----------------------------------------------------------------- This function will return differently sized favicons in different situations. I don't think that's what you want. Seems like it would be better to just figure out what dp you want here and call: Bitmap.createScaledBitmap(image, size, size, false);
Attachment #768599 - Flags: review?(wjohnston) → review-
(In reply to Wesley Johnston (:wesj) from comment #2) > Comment on attachment 768599 [details] [diff] [review] > Patch > > Review of attachment 768599 [details] [diff] [review]: > ----------------------------------------------------------------- > > This function will return differently sized favicons in different > situations. I don't think that's what you want. Seems like it would be > better to just figure out what dp you want here and call: > > Bitmap.createScaledBitmap(image, size, size, false); (I thought you like re-using code :P) I don't understand why Favicons is going to return different sizes. The Favicons is initialized with 32dp as maximum size. So, if the bitmap is larger than that, its going to scale it down to 32dp. This is the same code used in FaviconView too. Why would it work differently there and here?
Flags: needinfo?(wjohnston)
(In reply to Sriram Ramasubramanian [:sriram] from comment #3) > (In reply to Wesley Johnston (:wesj) from comment #2) > > Comment on attachment 768599 [details] [diff] [review] > > Patch > > > > Review of attachment 768599 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > This function will return differently sized favicons in different > > situations. I don't think that's what you want. Seems like it would be > > better to just figure out what dp you want here and call: > > > > Bitmap.createScaledBitmap(image, size, size, false); > > (I thought you like re-using code :P) > > I don't understand why Favicons is going to return different sizes. The > Favicons is initialized with 32dp as maximum size. So, if the bitmap is > larger than that, its going to scale it down to 32dp. This is the same code > used in FaviconView too. Why would it work differently there and here? Favicons.scaleImage returns different things depending on what was passed in (its a badly named function). That's because FaviconView displays differently for large vs small favicons. Since the icon you're passing in is just whatever we pulled out of the db, I don't think you have any idea what size it is.
Flags: needinfo?(wjohnston)
Comment on attachment 768599 [details] [diff] [review] Patch Review of attachment 768599 [details] [diff] [review]: ----------------------------------------------------------------- OK. Lets at least add a comment then. Something like: // Favicons.scaleIcon can return several different size favicons, // but will at least prevent this from being too large
Attachment #768599 - Flags: review- → review+
Assignee: nobody → sriram
Whiteboard: fixed-fig
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: