726001 - Decode ICO file format Favicons

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P2)

Description

[Aaron Train [:aaronmt]]

I was looking into why I dont get Favicons for the sites I visit.

Presently in all versions of native Fennec, Fennec will not display Favicons serving favicons using the ICO file format. This is due to use of the Android BitmapFactory [1] and ICO is not a supported media format [2]. In order to get Favicons served in this format, can we use Gecko?

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/Favicons.java#268
[2] http://developer.android.com/guide/appendix/media-formats.html 

XUL Firefox currently displays ICO's fine.

Comment 1

[Gian-Carlo Pascutto [:gcp]]

Decoding with Gecko is problematic because we want to show those images before it's really loaded. So we'd have to use it as a transcoding engine whenever we get an ICO and translate to PNG.

There are some indications that Android should really handle ICO as well:
http://stackoverflow.com/questions/2690503/can-bitmapfactory-decodefile-handle-ico-windows-icons-files

I'm digging through the Android source now, and for example decodeStream() basically calls SkImageDecoder in frameworks/base/core/jni/android/graphics/BitmapFactory.cpp

If we look further, we find:
external/skia/src/images/SkImageDecoder_libico.cpp

This strongly suggests we should be able to decode ICO when using decodeStream().

Comment 2

[Aaron Train [:aaronmt]]

Another example: http://amazon.ca uses http://www.amazon.ca/favicon.ico

Comment 3

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

Brian - did you try these example websites and _not_ see a problem?

Comment 4

[Brian Nicholson (:bnicholson)]

(In reply to Mark Finkle (:mfinkle) from comment #3)
> Brian - did you try these example websites and _not_ see a problem?

I didn't have any problems loading http://www.amazon.ca/favicon.ico from my own page, but http://androidandme.wpengine.netdna-cdn.com/wp-content/themes/android-and-me-v4/favicon.ico isn't working.

Comment 5

[Brian Nicholson (:bnicholson)]

Attachment: patch

For the amazon favicon I found a workaround posted here: http://groups.google.com/group/android-developers/browse_thread/thread/171b8bf35dbbed96/c3ec5f45436ceec8?lnk=raot. This may be a result of the same bug mentioned in the comments (http://code.google.com/p/android/issues/detail?id=6066), but for whatever reason, our existing fix isn't working.

The other icon, http://androidandme.wpengine.netdna-cdn.com/wp-content/themes/android-and-me-v4/favicon.ico, still doesn't work. There is likely a problem/incompatibility with this particular favicon and skia - which is why I was able to get amazon's favicon working when downloading locally, but the other favicon was still broken. The stock browser also fails to load this favicon, so it doesn't seem to be anything we're doing wrong.

Hopefully, whatever is breaking it is a rare case. If we really want the other favicon to work, we'll probably have to resort to including an additional image decoder or decoding the image in Gecko.

Comment 6

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

Comment on attachment 609975 [details] [diff] [review]
patch


>diff --git a/mobile/android/base/Favicons.java b/mobile/android/base/Favicons.java
Just some naming nits:

>+            InputStream instream = null;

instream -> contentStream

>+                HttpGet httpGet = new HttpGet(faviconUrl.toURI());

httpGet -> request

>+                BufferedHttpEntity bufHttpEntity = new BufferedHttpEntity(entity);

bufHttpEntity -> bufferedEntity

The internet seems to like this approach too. I am not ready to load up Gecko's image library yet, so let's just see how well this supports ICOs in the field.

Comment 7

[Brian Nicholson (:bnicholson)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/2f0536c9c497

Comment 8

[Brian Nicholson (:bnicholson)]

Comment on attachment 609975 [details] [diff] [review]
patch

[Approval Request Comment]
Fixes broken favicons on some sites.

Comment 9

[Alex Keybl [:akeybl]]

Comment on attachment 609975 [details] [diff] [review]
patch

[Triage Comment]
Mobile only - approved for Aurora 13.

Comment 10

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/2f0536c9c497

Comment 11

[Aaron Train [:aaronmt]]

Verified Fixed

Tested via: 
--
Samsung Galaxy Nexus (Android 4.0.2)
20120329040237
Inbound 8dd9a07db9a7

Comment 12

[Gian-Carlo Pascutto [:gcp]]

>Hopefully, whatever is breaking it is a rare case. If we really want the other 
>favicon to work, we'll probably have to resort to including an additional image 
>decoder or decoding the image in Gecko.

FWIW, sewardj was valgrinding for me when he ran into the following bug:

http://pastebin.mozilla.org/1553555

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Does this still need landing on mozilla-beta per comment 9?

Comment 14

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

(In reply to Ryan VanderMeulen from comment #13)
> Does this still need landing on mozilla-beta per comment 9?

No, it does not. Fx13 _was_ going to be the beta release, but Fx14 is now. This patch is already on Fx14, so we are good.