Closed Bug 1057629 Opened 5 years ago Closed 5 years ago

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

Categories

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

All
Android
defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 34

People

(Reporter: Margaret, Assigned: Margaret)

References

Details

Attachments

(2 files)

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.
Blocks: 1057634
Priority: -- → P1
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.
Attachment #8480282 - Flags: review?(bnicholson)
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.
Attachment #8480821 - Flags: review?(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.
Attachment #8480282 - Flags: review?(bnicholson) → review+
Attachment #8480821 - Flags: review?(bnicholson) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/54cafb17e8bc
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Depends on: 1061644
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.