Closed
Bug 1004517
Opened 11 years ago
Closed 4 years ago
Allow including local images for Home.panels imageUrl fields
Categories
(Firefox for Android Graveyard :: Awesomescreen, defect)
Tracking
(fennec+)
RESOLVED
INCOMPLETE
Tracking | Status | |
---|---|---|
fennec | + | --- |
People
(Reporter: Margaret, Unassigned, Mentored)
References
Details
(Whiteboard: [bad first bug][lang=java])
Attachments
(1 file)
(deleted),
patch
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•11 years ago
|
||
Actually, data URIs don't seem to work with Picasso either :(
Comment 2•10 years ago
|
||
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.
Reporter | ||
Comment 3•10 years ago
|
||
Lucas, would you want to mentor this bug? I think it would be a nice improvement to have :)
Flags: needinfo?(lucasr.at.mozilla)
Comment 4•10 years ago
|
||
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)
Updated•10 years ago
|
Assignee: nobody → lucasr.at.mozilla
Comment 5•10 years ago
|
||
Bump on this, as it blocks my idea for speeddial home panel.
Comment 6•10 years ago
|
||
(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?
Comment 7•10 years ago
|
||
As in typical speeddial, quickdial etc, thumbs of websites.
For example chrome://addonname/content/thumb1.png
Reporter | ||
Updated•10 years ago
|
Comment 8•10 years ago
|
||
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.
Updated•10 years ago
|
Assignee: lucasr.at.mozilla → nobody
Reporter | ||
Comment 9•10 years ago
|
||
(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]
Reporter | ||
Comment 10•10 years ago
|
||
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
tracking-fennec: --- → ?
Reporter | ||
Comment 11•9 years ago
|
||
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.
Reporter | ||
Comment 12•9 years ago
|
||
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)
Reporter | ||
Updated•9 years ago
|
Attachment #8602312 -
Attachment description: Support loading jar images in IageLoader → Support loading jar images in ImageLoader
Reporter | ||
Comment 13•9 years ago
|
||
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.
Reporter | ||
Comment 14•9 years ago
|
||
(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.
Updated•9 years ago
|
tracking-fennec: ? → +
Comment 15•9 years ago
|
||
Margaret: your JAR stream loader looks correct.
This sounds like your problem:
http://stackoverflow.com/a/13062314/22003
Status: NEW → ASSIGNED
Reporter | ||
Comment 16•9 years ago
|
||
(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)
Comment 17•9 years ago
|
||
I don't have the slightest idea. Did you check the stream returns the right bytes?
Flags: needinfo?(mh+mozilla)
Comment 18•9 years ago
|
||
Comment on attachment 8602312 [details] [diff] [review]
Support loading jar images in ImageLoader
Clearing this for now.
Attachment #8602312 -
Flags: feedback?(rnewman)
Reporter | ||
Updated•9 years ago
|
Assignee: margaret.leibovic → nobody
Status: ASSIGNED → NEW
Reporter | ||
Updated•9 years ago
|
Comment 19•4 years ago
|
||
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: 4 years ago
Resolution: --- → INCOMPLETE
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•