Pull search engines from open search plugins that we ship with Fennec

RESOLVED FIXED in Firefox 34

Status

defect
P1
normal
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: Margaret, Assigned: Margaret)

Tracking

Trunk
Firefox 34
All
Android
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

This should solve our l10n issues, as well as help us get icons for the search engines. It should also help with issues like getting the correct tablet-specific URL for Bing. And supporting open search will make it much easier to support new engines in the future.

Since our CSS injection is really the only non-open-search thing we're using right now, we can continue to add some hardcoded hacks for that, since I don't see that being a thing we will ship for real. In the real case, I think we would have some separate results endpoint for the search activity that serves up what we actually intend to show (or maybe one day a JSON endpoint that lets us make our own native results).
Assignee: nobody → margaret.leibovic
Priority: -- → P1
Blocks: 1056292
Blocks: 1056302
WIP here: https://github.com/mozilla/fennec-search/pull/65

I have the basics of open search parsing working (the bits that we're currently using in the search activity), and even wrote a little unit test for it.

Outstanding items:
* Getting real search plugin files from Fennec's jar (although we can keep a copy of some of them in the fennec-search repo for standalone development)
* Updating settings to pull search engine options from jar (this feels like a follow-up bug)
* Create localizable pref for default search engine (also follow-up material)
* Better support for various open search parameters, such as images (also follow-up)
(In reply to :Margaret Leibovic from comment #1)
> WIP here: https://github.com/mozilla/fennec-search/pull/65

Looks good! I put a couple of comments on the PR.

One more outsanding item: populating dynamic properties, eg:

   <Param name="rls" value="{moz:distributionID}:{moz:locale}:{moz:official}"/>

   http://lxr.mozilla.org/mozilla-central/source/mobile/locales/en-US/searchplugins/google.xml
Pull request here: https://github.com/mozilla/fennec-search/pull/65

I wrote a small unit test, but it just runs as part of the fennec-search repo.

There is definitely more work to do, but I think this patch would be good enough to land, since it is equivalent to what we currently ship in the search activity in Nightly. I decided to just hard-code pulling the en-US search plugins from the jar, but the search activity currently uses a hard-coded set of en-US search engines, so I think that's fine. In fact, the current set of search engine options is hard-coded into the settings code, so things would break if those plugins aren't included in certain locales.

Basically, I think I made a decent amount of progress with this patch, and I think it would be easier to iterate with it in the tree. It could also help eedens with his work on facets (bug 1056292).

Follow-up bugs I'll file (and fix!):

* Update settings to pull search engines from jar
* Get real locale for getting correct search plugin out of jar
* Set default search engine from localized pref
* Support for more search plugin parameters, including MozParams (we will probably need this for search deals)
* Better SearchEngine APIs (it bothers me that resultsUriForQuery and getSuggestionTemplate are inconsistently named)

I also want to update the fennec-search repo to use something other than resources for getting its own search plugins, since I had to comment that section of code out to get Fennec to compile.
Attachment #8477098 - Flags: review?(bnicholson)
Comment on attachment 8477098 [details] [diff] [review]
Use open search plugins for search engines

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

Looks good to me.

::: mobile/android/search/java/org/mozilla/search/providers/SearchEngine.java
@@ +60,5 @@
> +        this.identifier = identifier;
> +
> +        try {
> +            final XmlPullParser parser = Xml.newPullParser();
> +            parser.setFeature(XmlPullParser.FEATURE_PROCESS_NAMESPACES, false);

Docs say all features default to false, so I think you can remove this line.

@@ +65,5 @@
> +            parser.setInput(in, null);
> +            parser.nextTag();
> +            readSearchPlugin(parser);
> +        } finally {
> +            in.close();

It's weird that you're closing the InputStream in this constructor. I'd move the close to createEngine, where you first get the handle to the stream.

@@ +187,4 @@
>      }
>  
>      /**
>       * Create a uri strung that can be used to fetch the results page.

While here: s/strung/string/

@@ +192,5 @@
>       * @param query The user's query. This method will handle url escaping.
>       */
> +    public String resultsUriForQuery(String query) {
> +        if (resultsUri == null) {
> +            return "";

Are there any normal situations you would expect resultsUri to be null? If not, maybe we should assert/throw here. If resultsUri is null, it seems like something's busted. Throwing would be easier to track down an error than continuing with an empty string that results in weird behavior.

::: mobile/android/search/java/org/mozilla/search/providers/SearchEngineManager.java
@@ +120,5 @@
>  
>          try {
> +            return new SearchEngine(identifier, in);
> +        } catch (IOException e) {
> +            e.printStackTrace();

Nit: It's better to use "Log.e(<tag>, e);" on Android so the source of the error is clear.

@@ +130,3 @@
>      }
>  
> +    // TODO: Don't use resources, since those resources won't be available in Fennec.

So Fennec will be using SearchEngineManager? Where?

The search activity depends on Fennec, but not vice versa, so if Fennec needs to use this class, it might make sense to move it there instead of having it in the search package.
Attachment #8477098 - Flags: review?(bnicholson) → review+
(In reply to Brian Nicholson (:bnicholson) from comment #4)

> @@ +65,5 @@
> > +            parser.setInput(in, null);
> > +            parser.nextTag();
> > +            readSearchPlugin(parser);
> > +        } finally {
> > +            in.close();
> 
> It's weird that you're closing the InputStream in this constructor. I'd move
> the close to createEngine, where you first get the handle to the stream.

Good call, this was residual from the evolution of this patch.

> @@ +192,5 @@
> >       * @param query The user's query. This method will handle url escaping.
> >       */
> > +    public String resultsUriForQuery(String query) {
> > +        if (resultsUri == null) {
> > +            return "";
> 
> Are there any normal situations you would expect resultsUri to be null? If
> not, maybe we should assert/throw here. If resultsUri is null, it seems like
> something's busted. Throwing would be easier to track down an error than
> continuing with an empty string that results in weird behavior.

Also a good call. I forget why I added this check.

> @@ +130,3 @@
> >      }
> >  
> > +    // TODO: Don't use resources, since those resources won't be available in Fennec.
> 
> So Fennec will be using SearchEngineManager? Where?
> 
> The search activity depends on Fennec, but not vice versa, so if Fennec
> needs to use this class, it might make sense to move it there instead of
> having it in the search package.

I'm sorry, I think this comment is confusing. This current patch works when the search activity is built as part of Fennec (the main build situation we intend to support), but in the fennec-search repo I added some open search plugins as raw resouces to use for testing. I don't want to move those resources into mozilla-central, since they're not needed, and I don't want to introduce any confusion about where the search plugins came from.

So no, Fennec (as in GeckoApp/BrowserApp) won't be using SearchEngineManager. I was just talking about building as part of Fennec.
Blocks: 1057629
Blocks: 1057631
Merged PR: https://github.com/mozilla/fennec-search/commit/1613ca4cadf269d52d5ec05cfdfbe448bd2074c8

Waiting for fx-team to reopen to land there.

In addition to addressing review comments, I made two other changes:
* Escape query (this behavior was lost in the original patch)
* Use assets instead of resources for standalone search activity (avoids compile errors)
https://hg.mozilla.org/mozilla-central/rev/36bc6b13ac2c
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Blocks: 1059537
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.