We can't use the Favicons API as is because we'll have to support different types of images. For instance, some layouts will have larger images per item such as thumbnails. A few possible options: 1. Generalize part of the Favicons framework to handle other types of images. 2. Use something like Smoothie associated with some sort image disk/mem cache.

Comment on attachment 8368255 [details] [diff] [review] Add Picasso image loading library to the tree (r=nalexander) Picasso (https://github.com/square/picasso/), created by the Square team, is a pretty solid (and popular) open source image loading library for Android. What it gives us: - Image memory and disk cache (the sizes of these caches are dynamically defined at runtime depending on the device. We can always tweak them if we need to) - Off main thread image loading (from disk, memory and url) - View recycling handling i.e. auto-cancels requests for recycled views - Highly configurable framework i.e. we can implement our own cache, downloader, thread executor, and more if we need to. All this behind a pretty simple API. The library is released under the Apache License, Version 2.0 which is compatible with our stuff (we already have several pieces of code under the same license in our tree btw). I considered using Smoothie but that would means implementing the mem/disk/ caching and download routines on our own. I'd prefer to lean on more proven code so that we focus more on the home panel-specific features. For now, I'm not implementing any custom bits for the Picasso framework and simply sticking with the defaults (see following patches). Requesting feedback from Nick for the build system parts.

Comment on attachment 8368256 [details] [diff] [review] Use HomeItems DB constract in PanelListRow (r=margaret) We shouldn't be using URLColumns stuff on dynamic panel views anymore.

Comment on attachment 8368257 [details] [diff] [review] Add image_url column to HomeProvider's fake items (r=margaret) So that we can test image loading stuff with fake items.

Comment on attachment 8368258 [details] [diff] [review] Use Picasso to load images in PanelListRow and PanelGridItemView (r=margaret) Yep, it's that simple. Any configuration and custom components associated with Picasso should be encapsulated behind ImageLoader. For consistency, I stuck with the same API from Picasso to get a singleton instance (i.e. the with() method).

Comment on attachment 8368259 [details] [diff] [review] Remove unused imports from PanelListRow (r=margaret) Quick cleanup.

Comment on attachment 8368258 [details] [diff] [review] Use Picasso to load images in PanelListRow and PanelGridItemView (r=margaret) Review of attachment 8368258 [details] [diff] [review]: ----------------------------------------------------------------- Sweet.

Comment on attachment 8368255 [details] [diff] [review] Add Picasso image loading library to the tree (r=nalexander) Review of attachment 8368255 [details] [diff] [review]: ----------------------------------------------------------------- You were right, this was a simple review. In fact, I'm buoyed by how simple this is. It will be even cleaner when we move building the thirdparty jars to mobile/android/thirdparty/moz.build. One thing to note: this uses a *third* (or fourth?) HTTP stack (Gecko is one, Background Services another). More proxies, more problems. ::: mobile/android/base/Makefile.in @@ +61,5 @@ > ALL_JARS = \ > gecko-browser.jar \ > gecko-mozglue.jar \ > gecko-util.jar \ > + squareup-picasso.jar \ nit, open to discussion: version the JAR? ::: mobile/android/base/moz.build @@ +400,5 @@ > gbjar.generated_sources += sync_generated_java_files > gbjar.extra_jars = [ > 'gecko-mozglue.jar', > 'gecko-util.jar', > + 'squareup-picasso.jar', nit: version? @@ +440,5 @@ > + 'com/squareup/picasso/Transformation.java', > + 'com/squareup/picasso/UrlConnectionDownloader.java', > + 'com/squareup/picasso/Utils.java', > +] ] > +#spjar.javac_flags += ['-Xlint:all'] nit: I always prefer to ratchet the Xlint flags as high as possible, although it's less of an issue with thirdparty source.

Comment on attachment 8368255 [details] [diff] [review] Add Picasso image loading library to the tree (r=nalexander) Review of attachment 8368255 [details] [diff] [review]: ----------------------------------------------------------------- I feel like nalexander is more of an expert than me here, but I can give this my rubber stamp :)

(In reply to Nick Alexander :nalexander from comment #12) > Comment on attachment 8368255 [details] [diff] [review] > Add Picasso image loading library to the tree (r=nalexander) > > Review of attachment 8368255 [details] [diff] [review]: > ----------------------------------------------------------------- > > You were right, this was a simple review. In fact, I'm buoyed by how simple > this is. It will be even cleaner when we move building the thirdparty jars > to mobile/android/thirdparty/moz.build. > > One thing to note: this uses a *third* (or fourth?) HTTP stack (Gecko is > one, Background Services another). More proxies, more problems. The default "Downloader" implementation uses HttpUrlConnection which uses default HTTP stack on the Android. I filed bug 968897 to implement a Downloader that uses the same HTTP stack than, say, the Favicons framework (assuming this is the stack we've agreed to use on the Java side). > ::: mobile/android/base/Makefile.in > @@ +61,5 @@ > > ALL_JARS = \ > > gecko-browser.jar \ > > gecko-mozglue.jar \ > > gecko-util.jar \ > > + squareup-picasso.jar \ > > nit, open to discussion: version the JAR? Not sure I want to stick with specific versions as that might get misleading if, in the future, we land some hot fixes directly in our tree or cherry pick patches from upstream. > ::: mobile/android/base/moz.build > @@ +400,5 @@ > > gbjar.generated_sources += sync_generated_java_files > > gbjar.extra_jars = [ > > 'gecko-mozglue.jar', > > 'gecko-util.jar', > > + 'squareup-picasso.jar', > > nit: version? Ditto. > @@ +440,5 @@ > > + 'com/squareup/picasso/Transformation.java', > > + 'com/squareup/picasso/UrlConnectionDownloader.java', > > + 'com/squareup/picasso/Utils.java', > > +] ] > > +#spjar.javac_flags += ['-Xlint:all'] > > nit: I always prefer to ratchet the Xlint flags as high as possible, > although it's less of an issue with thirdparty source. Yeah, that should be fine for something like Picasso (I can tweak that if that becomes a problem in the future).

https://hg.mozilla.org/integration/fx-team/rev/342adfbeadc6 https://hg.mozilla.org/integration/fx-team/rev/144c332f9e4f https://hg.mozilla.org/integration/fx-team/rev/b85c89584af9 https://hg.mozilla.org/integration/fx-team/rev/4577b30df743 https://hg.mozilla.org/integration/fx-team/rev/f9c1a27f64bb

https://hg.mozilla.org/mozilla-central/rev/342adfbeadc6 https://hg.mozilla.org/mozilla-central/rev/144c332f9e4f https://hg.mozilla.org/mozilla-central/rev/b85c89584af9 https://hg.mozilla.org/mozilla-central/rev/4577b30df743 https://hg.mozilla.org/mozilla-central/rev/f9c1a27f64bb