Closed Bug 726001 Opened 8 years ago Closed 8 years ago

Decode ICO file format Favicons

Categories

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

ARM
Android
defect

Tracking

()

VERIFIED FIXED
Firefox 14
Tracking Status
firefox13 --- wontfix
firefox14 --- verified
blocking-fennec1.0 --- +
fennec 12+ ---

People

(Reporter: aaronmt, Assigned: bnicholson)

References

()

Details

(Whiteboard: parity-xul)

Attachments

(1 file)

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.
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().
tracking-fennec: --- → ?
Whiteboard: parity-xul
Assignee: nobody → gpascutto
tracking-fennec: ? → 12+
Priority: -- → P2
blocking-fennec1.0: --- → ?
blocking-fennec1.0: ? → +
Assignee: gpascutto → bnicholson
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.
Attached patch patchSplinter Review
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+
https://hg.mozilla.org/mozilla-central/rev/2f0536c9c497
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
Status: RESOLVED → VERIFIED
Keywords: checkin-needed
>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.
No longer blocks: ICODecoder
You need to log in before you can comment on or make changes to this bug.