1057629 - Use search plugins that ship with Fennec to populate options in search activity settings

Status: RESOLVED FIXED

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

Description

[:Margaret Leibovic]

Right now we have a hard-coded set of options. We should pull these options from the jar, providing the right options for the user's locale.

Comment 1

[:Margaret Leibovic]

Attachment: Use search plugins that ship with Fennec to populate options in search activity settings

This patch doesn't try to migrate existing prefs. This is an experimental feature in Nightly, so I think it's fine to reset users :)

One unfortunate thing I noticed is that we'll get into an endless crash cycle if you choose a search engine that doesn't support search suggestions, so I need to fix that before we land this. That makes me think that instead of crashing, if we have problems with a search engine, we should fail gracefully enough to give the user the opportunity to switch search engines.

I'll address that in a separate patch in this bug, since they should land together.

Comment 2

[:Margaret Leibovic]

PR here: https://github.com/mozilla/fennec-search/pull/73

Comment 3

[:Margaret Leibovic]

Attachment: (Part 2) Allow missing fields in search plugins

This allows both the suggestions URI and the results URI to be null. A search provider that doesn't have a results URI is pretty useless, but at least this won't crash and will allow the user to switch to a working search engine.

Comment 4

[Brian Nicholson (:bnicholson)]

Comment on attachment 8480282 [details] [diff] [review]
Use search plugins that ship with Fennec to populate options in search activity settings

Review of attachment 8480282 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/search/java/org/mozilla/search/SearchPreferenceActivity.java
@@ +112,5 @@
>  
> +        setUpSearchEnginePref();
> +    }
> +
> +    @SuppressWarnings("deprecation")

What's this for?

::: mobile/android/search/java/org/mozilla/search/providers/SearchEngine.java
@@ +44,5 @@
>                      "tag.type='text/css';" +
>                      "document.getElementsByTagName('head')[0].appendChild(tag);" +
>                      "tag.innerText='%s'})();";
>  
> +    public final String identifier;

Nit: Keep this private and make a getIdentifier getter.

::: mobile/android/search/java/org/mozilla/search/providers/SearchEngineManager.java
@@ +119,5 @@
> +            try {
> +                in = context.getResources().getAssets().open("engines/list.txt");
> +            } catch (IOException e) {
> +                Log.e(LOG_TAG, "Exception getting all search engines from assets", e);
> +                return null;

Might as well just throw here -- you'll get a NPE on the result when it calls "engines.size()", so it's better to just crash at the source of the error.

@@ +135,5 @@
> +                list.add(createEngine(identifier));
> +            }
> +        } catch (IOException e) {
> +            Log.e(LOG_TAG, "Exception creating all search engines from list", e);
> +            return null;

Same here.

Comment 5

[:Margaret Leibovic]

(In reply to Brian Nicholson (:bnicholson) from comment #4)
> Comment on attachment 8480282 [details] [diff] [review]
> Use search plugins that ship with Fennec to populate options in search
> activity settings
> 
> Review of attachment 8480282 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> :::
> mobile/android/search/java/org/mozilla/search/SearchPreferenceActivity.java
> @@ +112,5 @@
> >  
> > +        setUpSearchEnginePref();
> > +    }
> > +
> > +    @SuppressWarnings("deprecation")

This is for the findPreference call. I'm just moving over existing logic here, but we should probably file a bug to support a PreferenceFragment-based preference system. We just punted on that when we originally added this PreferenceActivity. We're using these deprecated APIs, since the new APIs are only available in API level 11+.

If you've seen our Fennec preferences code, it's a bit complicated, since we have different logic paths for pre/post-HC.

Comment 6

[:Margaret Leibovic]

https://github.com/mozilla/fennec-search/commit/11f629bc8cebde683c2a82646c1b74204f012e08
https://hg.mozilla.org/integration/fx-team/rev/54cafb17e8bc

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/54cafb17e8bc