885821 - Top Bookmarks thumbnails should use a centerCrop scaling

Status: RESOLVED FIXED

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

Description

[Sriram Ramasubramanian [:sriram]]

The top bookmarks (added by bug 882365) should use a centerCrop scaling if there's a bitmap and a center scaling if we are showing an icon.

Comment 1

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

This adds the CENTER scaleType while setting a favicon/"add bookmark" icon.

I somehow don't like this approach. To me, it's upto the BookmarkThumbnailView to decide on what scaling it should use. If it's going to show a dominant color background, it knows that the content doesn't fill its entire size, and hence use a CENTER scale. If not, it would be using the default CENTER_CROP style. My patch 3 in Bug 882365 uses this approach.

Comment 2

[Sriram Ramasubramanian [:sriram]]

Regarding using ThumbnailView as a parent class for BookmarkThumbnailView:

It's just extra work!
The ThumbnailHelper's width is set based on the width of BookmarkThumbnailView. The ThumbnailHelper's height and the height of this BookmarkThumbnailView are the same. This means that there isn't any need for complex matrix calculations done by ThumbnailView. The default ImageView takes care of scaling when set to centerCrop for this view.

In case of tabs tray, the thumbnail's width and height needn't be of same size as these thumbnails and ThumbnailHelper's. Hence there is a need to scale it and use a ThumbnailView. I am not doing extra work for this view unless there is a valid need for it.

Anyways, the above patch needs to be there immaterial of whether the parent of BookmarkThumbnailView or not.

Note: ThumbnailView doesn't care about scaling type while using a matrix. This will cause problems with favicons and other icons. That should be fixed before it is ever made a parent of BookmarkThumbnailView.

Comment 3

[Wesley Johnston (:wesj)]

(In reply to Sriram Ramasubramanian [:sriram] from comment #2)
> In case of tabs tray, the thumbnail's width and height needn't be of same
> size as these thumbnails and ThumbnailHelper's. Hence there is a need to
> scale it and use a ThumbnailView. I am not doing extra work for this view
> unless there is a valid need for it.

I'd argue that 1.) This work is already being done (badly) by Android:
http://androidxref.com/2.2.3/xref/frameworks/base/core/java/android/widget/ImageView.java#774

We're just doing it correctly.

2.) It protects future devs from having to deal with these issues. For instace, you have one here that will show up on upgrade where the saved thumbnail aspect ratio != your new thumbnails aspect ratio. But even if it didn't, I'd still push to use our correct code here so that they were more resilient to future changes, strange device sizes where maybe we can't get thumbnails at the aspect ratio you want, or general other bugs.

Comment 4

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

Had wrongly set CENTER to thumbnails while moving code from setBackgroundColor() in previous patch. Corrected it in this.

Comment 5

[Wesley Johnston (:wesj)]

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

Review of attachment 766162 [details] [diff] [review]:
-----------------------------------------------------------------

I'm filing a new bug about thumbnails being busted on upgrade.

Comment 6

[Sriram Ramasubramanian [:sriram]]

http://hg.mozilla.org/projects/fig/rev/ff8eb6acf9e5

Comment 7

[Ed Morley [:emorley]]

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