1059537 - Add support for parsing icons out of search plugin XML

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Search Activity, defect, P1)

Description

[:Margaret Leibovic]

This logic should get added in here:
https://github.com/mozilla/fennec-search/blob/master/app/src/main/java/org/mozilla/search/providers/SearchEngine.java#L80

And a testcase should get added in here:
https://github.com/mozilla/fennec-search/blob/master/app/src/androidTest/java/org/mozilla/search/test/SearchEngineTest.java

This is how the gecko search service does it:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/search/nsSearchService.js#1793

Comment 1

[:Margaret Leibovic]

Attachment: 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 2

[Brian Nicholson (:bnicholson)]

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'?

Comment 3

[:Margaret Leibovic]

(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.

Comment 4

[:Margaret Leibovic]

I decided to land a simpler version of this patch. We can explore adding support for getting different image sizes when we need them, since we'll also need to update the search plugins to do that.

https://hg.mozilla.org/integration/fx-team/rev/ca23c1819fc6
https://github.com/mozilla/fennec-search/commit/bc49fa0529b19a99254e1775aa1951de93615a61

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ca23c1819fc6