718665 - Parsing of data: URLs for menu item icons should be improved

Status: RESOLVED WORKSFORME

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

Description

[Wladimir Palant]

I checked how the data: URLs of menu item icons are processed in Fennec (http://hg.mozilla.org/mozilla-central/file/0e7d183d0d12/mobile/android/base/GeckoApp.java#l410) and was a bit horrified:

> byte[] raw = Base64.decode(item.icon.substring(22), Base64.DEFAULT);

This apparently assumes that the URL starts with something like "data:image/png;base64,", it will already fail if the mime type is image/jpeg - not to mention URL-encoded images (which should be rare). At the very least, this code should look for a comma instead of assuming a hardcoded length (the limitation to base64 should be documented of course). It might be even better to decode the data in Gecko by opening the data: channel synchronously and send an already decoded version to Java.

Comment 1

[Wladimir Palant]

A note about the context: this is the URL extension developers will pass in via NativeWindow.menu.add().

Comment 2

[Doug Turner (:dougt)]

><

Comment 3

[Sriram Ramasubramanian [:sriram]]

This piece of code is only for the icons specified by the addons to be added to the menu.
I believe the UX team has specified an order for the menu items, and only the first 5 of them has icons for them. The new menu items added by the addons will be appended to the list, hence will be at the end of the list. This will be seen only in overflow menu - which doesn't show any icons. Shall we remove this code and specify no icons for the menu items added by addons?

Comment 4

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

Based on discussions in IRC, it was felt that overflow menu cannot have icons at this point. Also, since there is no support for add-ons to add a menu item to the top of the list, it was decided to remove the code for now. Hereby, even if the add-on provides an icon to be shown, it wouldn't be shown based on Android's conventions.

Comment 5

[Matt Brubeck (:mbrubeck)]

Comment on attachment 594795 [details] [diff] [review]
Patch

I think we should remove the "icon" member from the ExtraMenuItem class while we're at it, and stop sending it in the Menu:Add message from browser.js.

We could also remove the aIcon parameter from NativeWindow.menu.add, although this would break our small number of existing add-ons.  Or, we could just document that it is currently unused.

Comment 6

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

Let's keep the arg in NativeWindow.menu.add but not use it. Maybe just rename it as aOptions so we can re-purpose it when we start using it again.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

This code is much better now, doesn't hard-code 22 any more. :)