Closed Bug 813346 Opened 9 years ago Closed 9 years ago

getBitmapFromDataURI() assumes fixed-length data URI header

Categories

(Firefox for Android Graveyard :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

(firefox19 fixed, firefox20 fixed)

RESOLVED FIXED
Firefox 20
Tracking Status
firefox19 --- fixed
firefox20 --- fixed

People

(Reporter: bnicholson, Assigned: bnicholson)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Blocks: 808663
No longer blocks: 812805
Duplicate of this bug: 814944
Reverts the change in bug 808663 that assumes a length of 22.
Assignee: nobody → bnicholson
Attachment #685365 - Flags: review?(wjohnston)
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).
Attachment #685365 - Flags: review?(wjohnston) → review-
Moved method into BitmapUtils. There were other places where we were decoding bitmaps from base64 strings, so I changed those too.
Attachment #685365 - Attachment is obsolete: true
Attachment #685752 - Flags: review?(wjohnston)
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.
Attachment #685752 - Flags: review?(wjohnston) → review+
Goods points. Landed with suggested changes:

http://hg.mozilla.org/integration/mozilla-inbound/rev/5b113b89bb19
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
Attachment #685752 - Flags: approval-mozilla-aurora?
Comment on attachment 685752 [details] [diff] [review]
Add getBitmapFromDataURI() to BitmapUtils

Low risk FF19 regression. Approving for Aurora.
Attachment #685752 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/5b113b89bb19
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 20
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.