813346 - getBitmapFromDataURI() assumes fixed-length data URI header

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Brian Nicholson (:bnicholson)]

In bug 808663, we changed the data URI substring to always start from the 22nd character. This breaks any data URIs that use a different MIME type, such as image/x-icon (see https://bugzilla.mozilla.org/show_bug.cgi?id=812805#c0 for one instance where this has thrown errors). We should change this back to start the substring after a comma character.

Comment 2

[Brian Nicholson (:bnicholson)]

Attachment: Don't assume fixed-length headers for search favicon data URIs

Reverts the change in bug 808663 that assumes a length of 22.

Comment 3

[Wesley Johnston (:wesj)]

Comment on attachment 685365 [details] [diff] [review]
Don't assume fixed-length headers for search favicon data URIs

This code is taken from:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoAppShell.java#854

Can you fix both (and probably combine them into some utility like I should have done to start with?). I think we have a BitmapUtils class in org.mozilla.gecko.gfx. That seems like a good place for this to me (and will make it easier to fix things if we ever want to throw in a non-base64 string).

Comment 4

[Brian Nicholson (:bnicholson)]

Attachment: Add getBitmapFromDataURI() to BitmapUtils

Moved method into BitmapUtils. There were other places where we were decoding bitmaps from base64 strings, so I changed those too.

Comment 5

[Wesley Johnston (:wesj)]

Comment on attachment 685752 [details] [diff] [review]
Add getBitmapFromDataURI() to BitmapUtils

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

Looks good. Those are all probably me copying and pasting :(

::: mobile/android/base/gfx/BitmapUtils.java
@@ +63,5 @@
>        hsv[2] = (float)maxV/10.0f;
>        return Color.HSVToColor(hsv);
>      }
> +
> +    public static Bitmap getBitmapFromDataURI(String dataURI) {

I'm trying to get in a better habit of documenting methods. Mind throwing something in here?

@@ +69,5 @@
> +        try {
> +            byte[] raw = Base64.decode(base64, Base64.DEFAULT);
> +            return BitmapFactory.decodeByteArray(raw, 0, raw.length);
> +        } catch (Exception e) {
> +            Log.i(LOGTAG, "exception while decoding bitmap: " + base64, e);

Should probably just spit out the dataURI here.

Comment 6

[Brian Nicholson (:bnicholson)]

Goods points. Landed with suggested changes:

http://hg.mozilla.org/integration/mozilla-inbound/rev/5b113b89bb19

Comment 7

[Brian Nicholson (:bnicholson)]

Comment on attachment 685752 [details] [diff] [review]
Add getBitmapFromDataURI() to BitmapUtils

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 808663 (Fx19)
User impact if declined: added search engines may be missing icons
Testing completed (on m-c, etc.): m-i
Risk to taking this patch (and alternatives if risky): low risk
String or UUID changes made by this patch: none

Comment 8

[Alex Keybl [:akeybl]]

Comment on attachment 685752 [details] [diff] [review]
Add getBitmapFromDataURI() to BitmapUtils

Low risk FF19 regression. Approving for Aurora.

Comment 9

[Brian Nicholson (:bnicholson)]

http://hg.mozilla.org/releases/mozilla-aurora/rev/db8d60352deb

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5b113b89bb19