Closed Bug 1004517 Opened 8 years ago Closed 1 year ago

Allow including local images for Home.panels imageUrl fields

Categories

(Firefox for Android Graveyard :: Awesomescreen, defect)

ARM
Android
defect
Not set
normal

Tracking

(fennec+)

RESOLVED INCOMPLETE
Tracking Status
fennec + ---

People

(Reporter: Margaret, Unassigned, Mentored)

References

Details

(Whiteboard: [bad first bug][lang=java])

Attachments

(1 file)

It looks like Picasso doesn't handle loading images from chrome:// URLs, and it also doesn't handle reading images from the jar.

Add-ons can include data URIs, but is there a better way we can allow them to bundle icons?

Our own BitmapUtils logic also has problems with chrome:// images (see bug 959896 and bug 993698), so this could also be part of a larger effort to make including images in add-ons less buggy and less confusing.
Actually, data URIs don't seem to work with Picasso either :(
Here are the type of URLs Picasso supports:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/thirdparty/com/squareup/picasso/BitmapHunter.java#193

Shouldn't be hard to extend this code to support more types.
Lucas, would you want to mentor this bug? I think it would be a nice improvement to have :)
Flags: needinfo?(lucasr.at.mozilla)
Posted a patch to upstream Picasso to enable us to implement custom image loading logic in Picasso:

https://github.com/square/picasso/pull/512

This will allow us, for example, to implement image loading based on our code in BitmapUtils.
Flags: needinfo?(lucasr.at.mozilla)
Assignee: nobody → lucasr.at.mozilla
Bump on this, as it blocks my idea for speeddial home panel.
(In reply to krzysztof113 from comment #5)
> Bump on this, as it blocks my idea for speeddial home panel.

What kind of local image loading do you need? Stuff stored on disk somewhere? base64 images?
As in typical speeddial, quickdial etc, thumbs of websites. 
For example chrome://addonname/content/thumb1.png
Blocks: 1046913
No longer blocks: lists
Just a quick update: the new APIs in Picasso mentioned in comment #4 have landed in master. We just need to wait for the 2.4 release now.
Assignee: lucasr.at.mozilla → nobody
(In reply to Lucas Rocha (:lucasr) from comment #8)
> Just a quick update: the new APIs in Picasso mentioned in comment #4 have
> landed in master. We just need to wait for the 2.4 release now.

This landed! http://lucasr.org/2014/09/23/new-features-in-picasso/

I can be the mentor here, but I haven't spent time looking into how this would work. Maybe lucasr can also be available to help answer some questions :)
Mentor: margaret.leibovic
Whiteboard: [bad first bug][lang=java]
We should raise the priority of this if we're thinking about encouraging home panels in distributions. It wouldn't be a great experience to need to download external images the first time the app is launched.
Assignee: nobody → margaret.leibovic
Blocks: home-panels
No longer blocks: 1046913
tracking-fennec: --- → ?
Depends on: 1161811
Blocks: 1162107
I filed bug 1161811 to update Picasso, but it turns out Picasso has picked up some new dependencies since we decided to include it in the tree, and I don't want us to pick up unnecessary bloat.

Luckily, lucasr already worked around this issue in the past:
https://bugzilla.mozilla.org/show_bug.cgi?id=1012462#c49

So I will attempt to enhance our own ImageLoader logic to support local images specified by add-ons.
This seems to work sometimes, but most of the time it fails to decode the images, and I see this error:
05-06 17:13:29.579  19314-19499/org.mozilla.fennec_leibovic D/skia﹕ --- SkImageDecoder::Factory returned null

I also tried using GeckoJarReader#getBitmap to pass a Bitmap to the response instead of an InputStream, but I just got a different decoding error in BitmapDrawable (our GeckoJarReader logic creates a BitmapDrawable to get a Bitmap, which seems less efficient than just passing an InputStream to Picasso and letting it figure things out).

Here's the add-on I'm using to test:
https://github.com/leibovic/speed-dial

Any idea what could be going wrong?
Attachment #8602312 - Flags: feedback?(rnewman)
Attachment #8602312 - Attachment description: Support loading jar images in IageLoader → Support loading jar images in ImageLoader
I just realized that if the immediate need for this is distributions, we could actully let distribution add-ons use  gecko.distribution:// URIs to pull images straight out of the distribution, since this is already supported.

I'll make a test distribution to see if this actually works.
(In reply to :Margaret Leibovic from comment #13)
> I just realized that if the immediate need for this is distributions, we
> could actully let distribution add-ons use  gecko.distribution:// URIs to
> pull images straight out of the distribution, since this is already
> supported.
> 
> I'll make a test distribution to see if this actually works.

So, good news, this does work, but it only works if I install the add-on after launching the browser (not as part of the distribution).

It appears that for some reason the home panel won't show up when installed from a bundled add-on. I do see the add-on installed, so I don't think this is bug 923581 (although that's something else to think about). I suspect this may be some race in initializing the home config and trying to install a new panel. I'll file a new bug to look into this.

In any case, although it would be great to write a patch for this bug to support images included in add-ons, there is a workaround for distributions.
tracking-fennec: ? → +
Margaret: your JAR stream loader looks correct.

This sounds like your problem:

http://stackoverflow.com/a/13062314/22003
Status: NEW → ASSIGNED
(In reply to Richard Newman [:rnewman] from comment #15)
> Margaret: your JAR stream loader looks correct.
> 
> This sounds like your problem:
> 
> http://stackoverflow.com/a/13062314/22003

I looked into resetting the InputStream as suggested here, but it's not supported here.

Looking into how we get this InputStream, we're getting it from GeckoJarReader, which is using NativeZip.

glandium, you wrote this code - do you know what could be going wrong?

For a bit of context, we're running into this error:
D/skia﹕ --- SkImageDecoder::Factory returned null

When trying to decode an InputStream here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/thirdparty/com/squareup/picasso/NetworkBitmapHunter.java#75

Maybe Picasso also just isn't designed to handle this type of input stream? However, I also tried getting a Bitmap directly from GeckoJarReader#getBitmap, since this Response object can also take a Bitmap instead of an InputStream, and I run into the same error in our own bitmap decoding logic.
Flags: needinfo?(mh+mozilla)
I don't have the slightest idea. Did you check the stream returns the right bytes?
Flags: needinfo?(mh+mozilla)
Comment on attachment 8602312 [details] [diff] [review]
Support loading jar images in ImageLoader

Clearing this for now.
Attachment #8602312 - Flags: feedback?(rnewman)
Assignee: margaret.leibovic → nobody
Status: ASSIGNED → NEW
Blocks: home-panel-addons
No longer blocks: home-panels
We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → INCOMPLETE
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.