Created attachment 8484600[details][diff][review]
Add support for parsing icons out of search plugin XML
PR (with testcase) here: https://github.com/mozilla/fennec-search/pull/81
To start, I just added support for the getIconURLBySize API that the search service has. I tried to keep this as similar to the search service as possible, so that we can just continue to port things over from there as needed.
We don't have any consumers of this yet, but we'll need this for bug 1049600.
Also, our search plugins in Fennec don't actually have images that correspond to the sizes that are included there, so we may need to look into fixing that (as well as adding larger images if we want them).

Comment on attachment 8484600[details][diff][review]
Add support for parsing icons out of search plugin XML
Review of attachment 8484600[details][diff][review]:
-----------------------------------------------------------------
It's not entirely clear to me how we're going to use this. What width and height will we be giving to getIconURLBySize()? Do we need to store all of the icons, or can we be smarter and just store the single icon that's closest to what we'll need as we parse?
That said, this is probably OK for now if you have a plan, and we can iterate when we actually use it.
::: mobile/android/search/java/org/mozilla/search/providers/SearchEngine.java
@@ +221,5 @@
> + * Height of the icon.
> + * @returns key string
> + */
> + private String getIconKey(int width, int height) {
> + final JSONObject obj = new JSONObject();
Using JSONObject just to get a key seems heavy. Can we just return something like 'width + "x" + height'?

(In reply to Brian Nicholson (:bnicholson) from comment #2)
> Comment on attachment 8484600[details][diff][review]
> Add support for parsing icons out of search plugin XML
>
> Review of attachment 8484600[details][diff][review]:
> -----------------------------------------------------------------
>
> It's not entirely clear to me how we're going to use this. What width and
> height will we be giving to getIconURLBySize()? Do we need to store all of
> the icons, or can we be smarter and just store the single icon that's
> closest to what we'll need as we parse?
>
> That said, this is probably OK for now if you have a plan, and we can
> iterate when we actually use it.
This was mostly me trying to be consistent with the search service, but you're right that this probably isn't going to be exactly what we'll want. Maybe I should update this to just store and return one icon, and we can expand it if necessary.
The fact that we'll be using the same search plugins that we use with Fennec means that we may need to support multiple icons in the XML, even if we only use one in the search activity. In antlam's most recent mock-ups for bug 1049600, there's a 24dp icon, but the image we currently include in the search plugins is 32px (but says that it's 16x16 for historic search service reasons).
I need to do some investigating to see how exactly this will all work, and how we could support multiple icon sizes. I also want to make sure we have a solution that works with different screen densities, since it seems like we don't really have that right now for the search engines in Fennec.
> ::: mobile/android/search/java/org/mozilla/search/providers/SearchEngine.java
> @@ +221,5 @@
> > + * Height of the icon.
> > + * @returns key string
> > + */
> > + private String getIconKey(int width, int height) {
> > + final JSONObject obj = new JSONObject();
>
> Using JSONObject just to get a key seems heavy. Can we just return something
> like 'width + "x" + height'?
Yeah... this was probably me just being a bit too literal about porting the logic over from the search service :)
I want to rethink this patch now.