808663 - Downscale awesome-bar row Favicons from 32dip to 16dip

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Aaron Train [:aaronmt]]

Currently the [1] ImageView used for awesome bar_rows and [2] in the browser-toolbar are upscaling 16x16 Favicons to 32x12 resulting in really poorly prominently displayed graphics in-product. 

These look extremely bad on high-end high-resolution devices like the Galaxy Note II where no other browser or Android application has this issue.

I have built Fennec with this value change and it is much nicer.

Can we please downscale Favicons to 16dip?

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/layout/awesomebar_row.xml#17

[2] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/layout/browser_toolbar.xml.in#75

Comment 1

[Aaron Train [:aaronmt]]

Attachment: Nightly (11/05) - Galaxy Note II

Comment 2

[Lucas Rocha (:lucasr)]

I think we should avoid varying favicon sizes as it wouldn't look good, especially on the awesome screen. So, my suggestion is to always show favicons in a 32dip *area* but avoid scaling them up by adding a surrounding border using the dominant color as background (or something like this). Ian, what do you think?

Comment 3

[Ian Barlow (:ibarlow)]

I see a few options here.

1. Find a way to not anti-alias icons when we scale them up, so that even if we use 16px favicons we can still show sharp pixels, not blurry edges: http://cl.ly/image/161r311M2l2W. However I've been trying to make this happen for about a year and a half though, and we can't seem to do it for whatever reason ;)

2. As Lucas suggested, scale favicons down to 16dip, but keep them in a 32x32 area. We might consider styling this with some kind of neutral background or border.

3. Building on #2, use 16dip favicons, and place them on a dominant colour background, choosing the colour in the same way we choose the "android home screen bookmark" background colour. 

-- 

Each of these options have their own strengths and tradeoffs, so ideally we should see if we can prototype all 3 of them, and then make a call on what is the best fix.

Comment 4

[Aaron Train [:aaronmt]]

Whatever works :-)

I just really cant stand seeing Favicons as is (for many months) right now, especially since they're so in-your-face in-product.

Comment 5

[Wesley Johnston (:wesj)]

I want to play with this.

Comment 6

[Wesley Johnston (:wesj)]

Attachment: Screenshot

Ian, what do you think of this? I'll post the patch in a sec, but I'm not sure this is great....

Comment 7

[Wesley Johnston (:wesj)]

Attachment: Patch

Patch to scale favicons without filtering them (nearest neighbor). I also applied this to the urlbar favicon too. I think this looks better, but that we might be good to reduce their size in the awesomescreen as well. Right now they're 32dip. Maybe 24dip? Easier to fix with this patch. I'll try it and post a screenshot!

Comment 8

[Ian Barlow (:ibarlow)]

(In reply to Wesley Johnston (:wesj) from comment #6)

> I'm not sure this is great....


Yeah. It's better, but not great either. Definitely still worth trying the options 2 and 3 from comment 3, where we see how 16dip favicons look.

Comment 9

[Wesley Johnston (:wesj)]

Hmm.. 24 isn't great either: https://dl.dropbox.com/u/72157/device-2012-11-06-123124.png

I'll play with some background things behind that.

Comment 10

[Ian Barlow (:ibarlow)]

I just opened the 24 one on my phone, and it actually looks half decent. Let's keep it on the table as one option.

Comment 11

[Brian Nicholson (:bnicholson)]

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

+        if (image != null) {
+            if (mFaviconSize > 0) {
+                image = Bitmap.createScaledBitmap(image, mFaviconSize, mFaviconSize, false);
+            }

Do we need this > 0 check? The constructor should always initialize mFaviconSize, so unless getDimension() is somehow returning a negative value, I don't see how mFaviconSize could ever be negative.

+    protected void updateFavicon(ImageView faviconView, Bitmap bitmap) {
+        if (bitmap == null) {
+            faviconView.setImageDrawable(null);
+        } else {
+            if (mFaviconSize > 0) {
+                bitmap = Bitmap.createScaledBitmap(bitmap, mFaviconSize, mFaviconSize, false);
+            }

Same here. The could would be a bit simpler if we didn't have these checks (and we wouldn't need the extra updateFavicon() method you're adding).

Comment 12

[Brian Nicholson (:bnicholson)]

(In reply to Brian Nicholson (:bnicholson) from comment #11)
> Same here. The could would be a bit simpler if we didn't have these checks

s/The could/The code/

Comment 13

[Wesley Johnston (:wesj)]

No we don't. I was just trying to use view.getWidth() for awhile, which is sometimes zero and will crash. So I left the safety checks in, but we shouldn't need them using the dimens approach.

Comment 14

[Wesley Johnston (:wesj)]

Playing with the homescreen background:

https://dl.dropbox.com/u/72157/home1.png - white background
https://dl.dropbox.com/u/72157/home2.png - no (well.. technically #3999)

We can reverse the overlay so that its below the icon too. For now its on top of the favicon.

Getting the dominant color is doable, but will take a bit more work to do right, we should calculate and store it when we first fetch the icon. Ideas ian?

Comment 15

[Brian Nicholson (:bnicholson)]

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

r+ with the mFaviconSize checks removed.

One other nit:

According to http://source.android.com/source/code-style.html#follow-field-naming-conventions, static field names should start with "s", so I'd rename it to sFaviconSize.

Comment 16

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

I assume we are waiting for Ian to report back on the preferred option?

Comment 17

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

... and can someone update the summary of this bug?

Comment 18

[Wesley Johnston (:wesj)]

This seems like an improvement regardless. I can land it alone and we can leave this open for any further work we want to do?

Just backed up with patches that have bounced that I'm trying to sort out.

Comment 19

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e40752d76915

Comment 20

[Ed Morley [:emorley]]

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

Comment 21

[Lucas Rocha (:lucasr)]

(In reply to Wesley Johnston (:wesj) from comment #14)
> Playing with the homescreen background:
> 
> https://dl.dropbox.com/u/72157/home1.png - white background
> https://dl.dropbox.com/u/72157/home2.png - no (well.. technically #3999)
> 
> We can reverse the overlay so that its below the icon too. For now its on
> top of the favicon.
> 
> Getting the dominant color is doable, but will take a bit more work to do
> right, we should calculate and store it when we first fetch the icon. Ideas
> ian?

Why not simply adding the borders and save the new image in the db instead of the original favicon? I like the white borders as a starting point btw. I'd probably only show it on smaller favicons though (16dip). No border is needed if the favicon can fill the 32dip space without scaling up.

Comment 22

[Aaron Train [:aaronmt]]

Attachment: Inbound (11/07) - Screenshot

Getting there. The filtering removal is good but the 32dp on all these 16x16's still look the same.

Comment 23

[Ian Barlow (:ibarlow)]

Hey guys -- I'm working on some simplified visual designs for the favicon backgrounds, will post when they're ready. 

In the mean time, of the screenshots so far, I am happiest with Wes's 24dip favicons with no background colour in comment 9. I think putting a background image behind them could work, but I agree with Lucas that I would use smaller 16dip icons in this case. 

Anyway designs are coming soon, just wanted to keep people up to date since there is a lot of discussion happening here.

Comment 24

[Wesley Johnston (:wesj)]

Attachment: Use 24dip icons

This moves the icons to 24dip. It also changes the scaleType since we're now doing the scaling manually ourselves (I want to ensure that Android doesn't do extra work).

Comment 25

[Wesley Johnston (:wesj)]

Attachment: WIP Patch for backgrounds

I'm just putting this up to save it or in case someone else wants to play with it. It adds background behind the icons (but not when they are saved like Lucas suggested above).

Comment 26

[Ian Barlow (:ibarlow)]

Hey folks, let's try this: I adapted the look we use in our add-ons and downloads panel, where we float an icon in the middle of a neutral coloured background - http://cl.ly/image/2f1J0m2P213G

favicons are 16dip in size, and the background colour is #e3e7ed

(I tried ones where we sampled the dominant colour, but it just looked too overwhelming)

Comment 27

[Ian Barlow (:ibarlow)]

Correction. After some deliberation in IRC, we are going to go with an alternate that I came up with earlier: http://cl.ly/image/2m143B0a3a2F

Still using 16dip favicons, but set inside a 32dip #fafbfc square with a 1dp corner radius.

Comment 28

[Wesley Johnston (:wesj)]

Attachment: Patch

White background. Screenshot on an SIII:

https://dl.dropbox.com/u/72157/home3.png

Comment 29

[Wesley Johnston (:wesj)]

Just noticed that our search engine icons for some reason aren't resized.

Comment 30

[Ian Barlow (:ibarlow)]

before you change that, can you send me a build? I'd be interested to see if having a different sized icon for search is useful at all.

Comment 31

[Brian Nicholson (:bnicholson)]

(In reply to Wesley Johnston (:wesj) from comment #29)
> Just noticed that our search engine icons for some reason aren't resized.

They use awesomebar_suggestion_row.xml, so you'll have to change that layout too.

Comment 32

[Wesley Johnston (:wesj)]

Attachment: Patch 2/2

Fixes up search icons. I needed a bitmap, and it seems a bit like a waste to use a Drawable for these anyway? A build without this patch is at:

https://dl.dropbox.com/u/72157/fennec-favicons.apk

Comment 33

[Brian Nicholson (:bnicholson)]

Comment on attachment 681150 [details] [diff] [review]
Patch 2/2

Have you tried this with different sites using the "Add Search Engine" feature for text boxes? Without the try/catch here, I wonder if we'd hit any problems if the favicon is corrupt (or if there's no favicon at all for the search engine).

Comment 34

[Ian Barlow (:ibarlow)]

This is looking good.

Request from myself and Stephen Horlander (who has been thinking about favicons for far longer than I have) -- Is there a way we can use the 16dip size, but if we have larger favicons available, to scale those to 32dip? 

While I worry that it might make the list look janky, Stephen makes a good point that we will continue to want larger favicons from web providers, and that any kind of evangelism or support we can provide might compel more people to make them.

Comment 35

[Wesley Johnston (:wesj)]

Attachment: Patch 3/2

Final fixup. This only scales up the icon if its bigger than 16px (not 16dip) and hides/shows the background image. I also added a try-catch around the search icon stuff because it was there before. Pulling and building to test that still.

Comment 36

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f3b9e6da96f2
https://hg.mozilla.org/integration/mozilla-inbound/rev/acd75423cc39
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d3acb8280f1

Comment 37

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f3b9e6da96f2
https://hg.mozilla.org/mozilla-central/rev/acd75423cc39
https://hg.mozilla.org/mozilla-central/rev/2d3acb8280f1

Comment 38

[Cristian Nicolae (:xti)]

The favicons are changed as expected on the latest Nightly and Aurora builds. Closing bug as verified fixed on:

Firefox 20.0a1 (2012-11-27)
Device: Galaxy Nexus
OS: Android 4.1.1