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

RESOLVED WORKSFORME

Status

()

Firefox for Android
General
P3
normal
RESOLVED WORKSFORME
6 years ago
4 years ago

People

(Reporter: Wladimir Palant (for Adblock Plus info Cc bugzilla@adblockplus.org), Assigned: sriram)

Tracking

Trunk
ARM
Android
Points:
---

Firefox Tracking Flags

(fennec+)

Details

Attachments

(1 attachment)

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.
A note about the context: this is the URL extension developers will pass in via NativeWindow.menu.add().

Comment 2

6 years ago
><
Assignee: nobody → sriram
tracking-fennec: --- → +
Priority: -- → P3
(Assignee)

Comment 3

6 years ago
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?
(Assignee)

Comment 4

6 years ago
Created attachment 594795 [details] [diff] [review]
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.
Attachment #594795 - Flags: review?(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.
Attachment #594795 - Flags: review?(mbrubeck) → review-
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.
This code is much better now, doesn't hard-code 22 any more. :)
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.