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)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 26
People
(Reporter: sriram, Assigned: sriram)
References
Details
(Whiteboard: fixed-fig)
Attachments
(1 file)
1.90 KB,
patch
|
wesj
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•12 years ago
|
||
Attachment #768599 -
Flags: review?(wjohnston)
Comment 2•12 years ago
|
||
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-
Assignee | ||
Comment 3•12 years ago
|
||
(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)
Comment 4•12 years ago
|
||
(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 5•12 years ago
|
||
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 | ||
Comment 6•12 years ago
|
||
Assignee: nobody → sriram
Whiteboard: fixed-fig
Comment 7•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
Updated•5 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•