Closed Bug 726001 Opened 8 years ago Closed 8 years ago
Decode ICO file format Favicons
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  and ICO is not a supported media format . In order to get Favicons served in this format, can we use Gecko?  http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/Favicons.java#268  http://developer.android.com/guide/appendix/media-formats.html XUL Firefox currently displays ICO's fine.
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().
Assignee: nobody → gpascutto
tracking-fennec: ? → 12+
Priority: -- → P2
Brian - did you try these example websites and _not_ see a problem?
(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.
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.
Attachment #609975 - Flags: review?(mark.finkle)
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.
Attachment #609975 - Flags: review?(mark.finkle) → review+
Comment on attachment 609975 [details] [diff] [review] patch [Approval Request Comment] Fixes broken favicons on some sites.
Attachment #609975 - Flags: approval-mozilla-aurora?
Comment on attachment 609975 [details] [diff] [review] patch [Triage Comment] Mobile only - approved for Aurora 13.
Attachment #609975 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 14
Verified Fixed Tested via: -- Samsung Galaxy Nexus (Android 4.0.2) 20120329040237 Inbound 8dd9a07db9a7
>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
Does this still need landing on mozilla-beta per comment 9?
(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.
You need to log in before you can comment on or make changes to this bug.