1091728 - Support distribution search engines in search activity

Status: RESOLVED FIXED

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

Description

[Karen Rudnitski [:kar]]

Could we please test to ensure that if a default search provider is listed in a distribution file, that this becomes the default not only in the URL bar search but ALSO the Search Activity widget as well?

We should check this happens correctly if the distribution is done OTA or pre-installed. 

Thanks!

Comment 1

[:Margaret Leibovic]

Yep, this doesn't work. This is going to require some work in SearchEngineManager to figure out how to get the distribution search plugin.

Comment 2

[:Margaret Leibovic]

So, the way this works in Firefox is that the search service asks DirectoryProvider for the right search plugin files. We have our own DirectoryProvider implementation that adds the files from the distribution directory:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/DirectoryProvider.js#104

To do this in the search activity, we'll want to just figure out the right path to the distribution search plugin XML, and look there.

rnewman, does the Distribution class depend on BrowserApp/GeckoApp? Can I use it directly from the search activity code? I don't want to duplicate the logic to look for files in different places. Also, in the case of downloadable distributions, we would need that download to happen before we could actually use the distribution.

I'm also thinking about the edge case where the user launches the search activity before launching the browser on first run. Worst case, we'll fall back to the regular default (Yahoo!) for that first run, but it would be nice to have this work properly every time.

Comment 3

[:Margaret Leibovic]

Well, another issue I discovered here is that we aren't even trying to use the correct default engine identifier here. This is caused by the fact that we store the search activity default in SharedPrefereces, and we only update it from the gecko side when the default *changes*:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#7079

We do migrate this value on app update:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#880

However, this makes me suspect that we're not even using the correct default engine for other locales. If this SharedPreference isn't set, we fall back to Yahoo!, so we wouldn't notice this in en-US builds. This is a problem.

Comment 4

[Richard Newman [:rnewman]]

(In reply to :Margaret Leibovic from comment #2)

> rnewman, does the Distribution class depend on BrowserApp/GeckoApp? Can I
> use it directly from the search activity code?

It doesn't, and yes, you can. You'd have to init it, just as BrowserApp does, but otherwise it should work fine.

The only risk I see is that we haven't tested the first-run post-install intent handler via the search activity, but we should do that anyway.

> I don't want to duplicate the
> logic to look for files in different places.

Alas, you'll be somewhat reimplementing nsSearchService regardless, no?

Comment 5

[Richard Newman [:rnewman]]

(In reply to :Margaret Leibovic from comment #3)

> However, this makes me suspect that we're not even using the correct default
> engine for other locales. If this SharedPreference isn't set, we fall back
> to Yahoo!, so we wouldn't notice this in en-US builds. This is a problem.

In the absence of Gecko having run, and cached its computed default, I think you pretty much have to bite the bullet and implement the list.txt etc. parsing yourself.

This shouldn't be too difficult, at least.

Comment 6

[:Margaret Leibovic]

(In reply to Richard Newman [:rnewman] from comment #5)
> (In reply to :Margaret Leibovic from comment #3)
> 
> > However, this makes me suspect that we're not even using the correct default
> > engine for other locales. If this SharedPreference isn't set, we fall back
> > to Yahoo!, so we wouldn't notice this in en-US builds. This is a problem.
> 
> In the absence of Gecko having run, and cached its computed default, I think
> you pretty much have to bite the bullet and implement the list.txt etc.
> parsing yourself.
> 
> This shouldn't be too difficult, at least.

We do already have the list.txt parsing, but the default comes from a gecko pref, not from list.txt:
http://mxr.mozilla.org/mozilla-central/source/mobile/locales/en-US/chrome/region.properties#5

I don't know how we'd be able to find the right default before Gecko ever runs. Maybe we should disable the search activity until Firefox has been launched at least once? Is that possible?

Comment 7

[Richard Newman [:rnewman]]

(In reply to :Margaret Leibovic from comment #6)

> I don't know how we'd be able to find the right default before Gecko ever
> runs.

Sorry, I was imprecise; by "list.txt etc." I meant "all the gubbins that the search service does to come up with the list of engines".

In a sense, that it's a simple value in region.properties is nice: it's easy to parse!


You'd use GeckoJarReader to dig into omni.ja, just as you already do for list.txt, using the current locale -- again, just as you already do -- and dig that out.

At that point you've read the list, read the engines, determine the locale, and pulled out the default engine order and the default engine.

You're already planning to look in the distribution, so you can layer any search-related distro stuff on top.

That's everything the search service does, so the only thing you'd have to do beyond that is reflect any changes the user makes. The existing mirrored pref takes care of that.


> Maybe we should disable the search activity until Firefox has been
> launched at least once? Is that possible?

Not really; it's a manifest entry. We could have it launch BrowserApp, or we could have it tickle Gecko and hope for the best, or we could do what I suggest above and reimplement a small part of the search service in Java.

Comment 8

[:Margaret Leibovic]

So, the first part of this is that we need to pick the right default search engine.

If the user ever changes their search engine (or they updated their browser from an old version of Firefox such that we perform a migration), we'll set the SharedPreference from gecko, so we won't need to do anything in this case.

Otherwise, if we don't see a SharedPreference in SearchEngineManager.getEngineFromPrefs, we need to be clever to figure out what the correct default value should be, and then we should store that value in the preference for the future.

In the case of a distribution, we'll want to get the "browser.search.defaultenginename" our of preferences.json. However, this is in "LocalizablePreferences", which means that we'll want to implement some logic to check for the right preference for the right locale. We can look at this logic for inspiration, but it will be simpler for us if we're just looking for this one single pref:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#7662

In the case that there isn't a distribution, we'll want to read this value out of region.properties, and we'll also have to make sure that we're poking into the correct locale file. We already have logic for locale paths in getInputStreamFromJar, so this shouldn't be too hard.

Okay, now we have the correct engine identifier.

The second thing we'll need to do is figure out how to get the correct search plugin XML for that identifier. Right now we first look in the jar for default engines, followed by the profile for user-added engines. We'll want to add some logic above that to first look in the distribution directory if a distribution exists.

rnewman, does this sound like a good plan? Did I miss anything?

Comment 9

[Richard Newman [:rnewman]]

That sounds like a good plan.

There is possibly some pain around prefs that are *sometimes* localized, so be prepared to be confounded there (as I was in Bug 1091803). Using Java rather than nsIPrefService probably makes this easier to get right!

Probably best if this is something we comment the hell out of, too. But you're good at that :D

Comment 10

[:Margaret Leibovic]

Attachment: WIP - Use correct gecko default search engine in search activity

It turns out that we landed code in bug 1065306 to generate a raw resource file that holds the default engine name from region.properties, but we never wrote a patch to actually use that!

I still need to write the logic to actually use a distribution search plugin, but this is correctly finding and using the default engine from region.properties.

The main concern I have with this patch is that we store an engine *name* for the default preference in Gecko, but we want an identifier that corresponds to an .xml file name in the search activity. For third-party plugins that the user adds, we have logic to generate this file name, but for shipped search plugins we mostly rely on convention. Hopefully in practice this won't be much of a problem because distributions can test the search activity to make sure it works with whatever engine they try to add.

Comment 11

[Richard Newman [:rnewman]]

Comment on attachment 8518585 [details] [diff] [review]
WIP - Use correct gecko default search engine in search activity

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

::: mobile/android/search/java/org/mozilla/search/providers/SearchEngineManager.java
@@ +37,5 @@
>  
>  public class SearchEngineManager implements SharedPreferences.OnSharedPreferenceChangeListener {
> +    private static final String LOG_TAG = "GeckoSearchEngineManager";
> +
> +    // Default search engine pref in Gecko.

Worth being explicit that the Gecko-ness is about the pref, not about the search engine:

// Gecko pref that defines the name of the default search engine.

@@ +110,4 @@
>              @Override
> +            public void run() {
> +                // First look in shared preferences.
> +                final String identifier = GeckoSharedPrefs.forApp(context).getString(Constants.PREF_SEARCH_ENGINE_KEY, null);

Worth noting: this is a per-app pref, but the Gecko pref that defines this, strictly speaking, is per-profile.

In the future world where we handle multiple profiles (or even right now in Guest Mode), we can expect this value to flip-flop as profiles switch, no?

@@ +121,4 @@
>                      }
>                  }
>  
> +                // If a shared preference isn't set, we need to poke into some Gecko files.

I'd like to see this section of code lifted out into a testable method.

@@ +127,5 @@
> +                    @Override
> +                    public void run() {
> +                        // First, look for the default search engine in a distribution.
> +                        String identifier = getIdentifierFromDistribution(distribution);
> +                        if (TextUtils.isEmpty(identifier)) {

Have these getIdentifier methods return null for empty strings?

Or is the emptiness check actually too weak, and we should be comparing the value against the set of search engines for validity?

(And presumably we can't do that at this moment in time, so we should really be returning a sorted list and picking the first valid one when we're reading the search engines themselves…)

@@ +129,5 @@
> +                        // First, look for the default search engine in a distribution.
> +                        String identifier = getIdentifierFromDistribution(distribution);
> +                        if (TextUtils.isEmpty(identifier)) {
> +                            // Otherwise, get the default engine for the locale.
> +                            identifier = getIdenitiferFromResources();

Typo.

@@ +134,5 @@
> +                        }
> +                        if (TextUtils.isEmpty(identifier)) {
> +                            // Something has gone really wrong if we get here.
> +                            Log.e(LOG_TAG, "Couldn't find a default search engine.");
> +                            return;

What happens if the callback is never called?

@@ +137,5 @@
> +                            Log.e(LOG_TAG, "Couldn't find a default search engine.");
> +                            return;
> +                        }
> +
> +                        runCallback(createEngine(identifier), callback);

Remember (see above), createEngine can throw. You probably don't want to kill the background thread under any circumstances!

@@ +142,5 @@
> +
> +                        // Store the identifier for the future.
> +                        final SharedPreferences.Editor editor = GeckoSharedPrefs.forApp(context).edit();
> +                        editor.putString(Constants.PREF_SEARCH_ENGINE_KEY, identifier);
> +                        editor.apply();

Use chaining and stack here:

  GeckoSharedPrefs.forApp(context)
                  .edit()
                  .putString(…)
                  .apply();

@@ +149,5 @@
> +            }
> +        });
> +    }
> +
> +    private String getIdentifierFromDistribution(Distribution distribution) {

Note that this needs to be called after the distribution is ready.

@@ +159,5 @@
> +        try {
> +            final JSONObject all = new JSONObject(getFileContents(prefFile));
> +
> +            // First, check to see if there's a locale-specific override.
> +            final String languageTag = BrowserLocaleManager.getLanguageTag(Locale.getDefault());

Are you sure that BLM has already fixed up the current locale at this point?

(It should have if you're calling this from the search activity, and of course you can assume that there's no language override set yet, but worth documenting this.)

@@ +301,5 @@
>      }
> +
> +    /**
> +     * Shortcut to slurp a file without messing around with streams.
> +     * Copied from Distribution.java.

Move it to FileUtils?

@@ +310,5 @@
> +            scanner = new Scanner(file, "UTF-8");
> +            return scanner.useDelimiter("\\A").next();
> +        } finally {
> +            if (scanner != null) {
> +                scanner.close();

It looks like this whole null check thing can go if the constructor call is lifted out of the try block.

@@ +321,5 @@
> +     * This is the logic that is used to create file names (identifiers) for search
> +     * plugin XML serialized to disk.
> +     *
> +     * We're making an assumption that default engines shipped with the distribution
> +     * follow the file name pattern as dynamically generated plugins.

We should validate this assumption against the l10n repos.

Further, we should probably avoid this. There aren't many XML files; by all means guess (and validate), but be prepared to scan them and do a name match.

Comment 12

[:Margaret Leibovic]

(In reply to Richard Newman [:rnewman] from comment #11)

> @@ +110,4 @@
> >              @Override
> > +            public void run() {
> > +                // First look in shared preferences.
> > +                final String identifier = GeckoSharedPrefs.forApp(context).getString(Constants.PREF_SEARCH_ENGINE_KEY, null);
> 
> Worth noting: this is a per-app pref, but the Gecko pref that defines this,
> strictly speaking, is per-profile.
> 
> In the future world where we handle multiple profiles (or even right now in
> Guest Mode), we can expect this value to flip-flop as profiles switch, no?

This is a good point, and something we should think about. I think it's fine to keep this pref in sync with the default profile for now, but you point about guest profiles actually uncovers a bug - if a user in a guest profile changes the default search engine, they'll end up changing the default for the search activity. We should file a bug to add an (!guestMode) check around that logic.

> @@ +121,4 @@
> >                      }
> >                  }
> >  
> > +                // If a shared preference isn't set, we need to poke into some Gecko files.
> 
> I'd like to see this section of code lifted out into a testable method.

Yes, I need to write some tests for this logic... I filed bug 1072429 about moving the JUnit test I wrote for the standlone search activity into m-c, but I haven't fixed that yet. I should do that.

> @@ +127,5 @@
> > +                    @Override
> > +                    public void run() {
> > +                        // First, look for the default search engine in a distribution.
> > +                        String identifier = getIdentifierFromDistribution(distribution);
> > +                        if (TextUtils.isEmpty(identifier)) {
> 
> Have these getIdentifier methods return null for empty strings?
> 
> Or is the emptiness check actually too weak, and we should be comparing the
> value against the set of search engines for validity?
> 
> (And presumably we can't do that at this moment in time, so we should really
> be returning a sorted list and picking the first valid one when we're
> reading the search engines themselves…)

Hm, yeah, I guess the 

> @@ +129,5 @@
> > +                        // First, look for the default search engine in a distribution.
> > +                        String identifier = getIdentifierFromDistribution(distribution);
> > +                        if (TextUtils.isEmpty(identifier)) {
> > +                            // Otherwise, get the default engine for the locale.
> > +                            identifier = getIdenitiferFromResources();
> 
> Typo.
> 
> @@ +134,5 @@
> > +                        }
> > +                        if (TextUtils.isEmpty(identifier)) {
> > +                            // Something has gone really wrong if we get here.
> > +                            Log.e(LOG_TAG, "Couldn't find a default search engine.");
> > +                            return;
> 
> What happens if the callback is never called?

The UI will never finish being set up, since there would be no search engine for it to use. If we really can't find a search engine, maybe we should show a message to the user somehow, but we really should program this such that that will never be the case. So maybe I shouldn't even have this if check, we should just let the app blow up if this ever happens, since it really is a bug.

> @@ +137,5 @@
> > +                            Log.e(LOG_TAG, "Couldn't find a default search engine.");
> > +                            return;
> > +                        }
> > +
> > +                        runCallback(createEngine(identifier), callback);
> 
> Remember (see above), createEngine can throw. You probably don't want to
> kill the background thread under any circumstances!

Yeah, this is especially true if we're trying to guess the right file name from the engine name. We should be careful here.

> @@ +149,5 @@
> > +            }
> > +        });
> > +    }
> > +
> > +    private String getIdentifierFromDistribution(Distribution distribution) {
> 
> Note that this needs to be called after the distribution is ready.
> 
> @@ +159,5 @@
> > +        try {
> > +            final JSONObject all = new JSONObject(getFileContents(prefFile));
> > +
> > +            // First, check to see if there's a locale-specific override.
> > +            final String languageTag = BrowserLocaleManager.getLanguageTag(Locale.getDefault());
> 
> Are you sure that BLM has already fixed up the current locale at this point?
> 
> (It should have if you're calling this from the search activity, and of
> course you can assume that there's no language override set yet, but worth
> documenting this.)

I didn't know this was something to be concerned about. Will document!

> @@ +321,5 @@
> > +     * This is the logic that is used to create file names (identifiers) for search
> > +     * plugin XML serialized to disk.
> > +     *
> > +     * We're making an assumption that default engines shipped with the distribution
> > +     * follow the file name pattern as dynamically generated plugins.
> 
> We should validate this assumption against the l10n repos.

Yes. I'm not so worried about distributions, since we've been the ones creating distribution files for our partners (and there are few distributions, and this can be tested), but it would be good to check on the existing plugins in the l10n repos.

> Further, we should probably avoid this. There aren't many XML files; by all
> means guess (and validate), but be prepared to scan them and do a name match.

In theory, we could do what the search service does, which is read and parse all the plugins, then look at them for a name match.

Comment 13

[:Margaret Leibovic]

While working on this, I was curious about how the Amazon.com plugin was even currently working in the search activity if you set it to your default engine, since you cannot derive "amazondotcom.xml" from the "Amazon.com" engine name.

Then I noticed that we do save the proper search engine identifier in the shared preference when you switch your default engine:
http://hg.mozilla.org/mozilla-central/file/f2a91c7332be/mobile/android/chrome/content/browser.js#l7030

So, in practice, we really only need to care about *default* engine XML file names matching what we expect.

But yeah, this is all so fragile, we should probably bite the bullet and have fallback code to read the files.

Comment 14

[:Margaret Leibovic]

Attachment: WIP v2 - Use correct gecko default search engine in search activity

Instead of trying to guess the file name from the engine name, this patch actually iterates through the files listed in list.txt looking for a name match.

The only thing left to do here is to hook up looking for the distribution search engines in the right place.

For distributions, we should look in both the distribution /searchplugins directory, as well as in the jar, since it would work on the gecko side for a distribution to change the default engine to one of the ones we ship (although I'm not sure how often that would happen in practice).

Comment 15

[:Margaret Leibovic]

Attachment: WIP v3 - Use correct gecko default search engine in search activity

Updated to reflect some more progress... at this point the main thing missing is getting a distribution engine out from a stored shared pref.

Comment 16

[Richard Newman [:rnewman]]

Comment on attachment 8520331 [details] [diff] [review]
WIP v3 - Use correct gecko default search engine in search activity

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

::: mobile/android/search/java/org/mozilla/search/providers/SearchEngineManager.java
@@ +142,5 @@
> +        distribution.addOnDistributionReadyCallback(new Runnable() {
> +            @Override
> +            public void run() {
> +                // First, look for the default search engine in a distribution.
> +                SearchEngine engine = getEngineFromDistribution(distribution);

getDefaultEngineFromDistribution?

Also, I think you need a guard call to distribution.exists().

@@ +152,5 @@
> +                runCallback(engine, callback);
> +
> +                // Store the identifier for the future.
> +                GeckoSharedPrefs.forApp(context)
> +                        .edit()

Small nit: align the '.'s.

@@ +246,5 @@
> +        for (int i = 0; i < files.length; i++) {
> +            try {
> +                final FileInputStream fis = new FileInputStream(files[i]);
> +                // TODO: Store an identifier that corresponds to file name? We need a way
> +                // to get the engine again when we store this in a shared preference...

This looks like something you need to fix before this lands. Am I wrong?

@@ +325,4 @@
>          InputStream in = getInputStreamFromJar(identifier + ".xml");
>  
>          if (in == null) {
> +            // Third-party search plugins installed by the user will be locatd in the profile.

located

@@ +330,4 @@
>          }
>  
> +        // TODO: Look for engine in distribution directory.
> +        // We'll probably need to store an identifier that corresponds we the file name.

Is this comment still valid? It probably is from a later-than-checking standpoint, but if not...

@@ +341,5 @@
> +
> +    /**
> +     * Creates a SearchEngine instance from an InputStream.
> +     *
> +     * This method closes the stream after it is done reading it. 

Nit: trailing whitespace.

@@ +349,5 @@
>              try {
>                  return new SearchEngine(identifier, in);
>              } finally {
>                  in.close();
>              }

You can just attach the `finally` to the outermost try.

Comment 17

[:Margaret Leibovic]

(In reply to Richard Newman [:rnewman] from comment #16)

> @@ +246,5 @@
> > +        for (int i = 0; i < files.length; i++) {
> > +            try {
> > +                final FileInputStream fis = new FileInputStream(files[i]);
> > +                // TODO: Store an identifier that corresponds to file name? We need a way
> > +                // to get the engine again when we store this in a shared preference...
> 
> This looks like something you need to fix before this lands. Am I wrong?

Yes, I still need to figure out how I'm going to deal with these engines that are in the distribution. In the search activity here, we're always using the identifier as the file name, so I was thinking of just storing the file name for the distribution engines as well, but I just realized that's going to break if the user manually sets one of these engines as their default:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#7009

I'll think about this more today and come up with a solution. Instead of storing an "identifier" for the search engine, it might be more straightforward (and honest) to rename this to "fileName", or even just store where we can find the plugin.

Comment 18

[:Margaret Leibovic]

Attachment: Use correct gecko default search engine in search activity

This has changed quite a bit since my last WIP, but this is finally a patch that's actually correct.

I decided that we should just store the engine name the same way the search service does because it's just too hard (or impossible) to keep track of a magical identifier. I kept the `identifier` field in SearchEngine because we still want to use that for FHR, but now it will be consistent with Fennec searches and only include that identifier for engines we ship (and that's all we really care about there anyway).

I contemplated trying to optimize this by storing a pref to keep track of where the plugin is located, in addition to the default engine name, but I am seeing absolutely zero delay on my phone for a third-party search engine (although I am using an N4, so I should test with some slower devices). In the worst case, this is only going to do as much work as the search service does when we launch the browser, so I feel like this is probably not something that needs to be optimized. Correctness > speed.

Also, we're going to want to uplift this to Aurora, so I want to try to find the simplest correct patch that we can land (unfortunately I think this big patch is the "simplest" thing).

Comment 19

[:Margaret Leibovic]

I should also mention that this may cause some problems for Nightly/Aurora users who have changed their default engine... we have logic in onAppUpdated to migrate the default pref if users have changed it before we shipped the search activity, but those Nightly/Aurora users won't hit that.

I'm fine with that, since in the worst case we just end up showing the default search engine in the search activity, and as a workaround those users can always just toggle their defaults in the search settings. However, I'm probably biased because I would need to implement the solution if we want to address this issue, so I'm open to people arguing with me.

+bnicholson, since he reviewed that migration logic.

Comment 20

[Richard Newman [:rnewman]]

Comment on attachment 8521839 [details] [diff] [review]
Use correct gecko default search engine in search activity

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

This seems sane to me.

::: mobile/android/base/util/FileUtils.java
@@ +83,5 @@
>          return file.delete();
>      }
> +
> +    // Shortcut to slurp a file without messing around with streams.
> +    public static String getFileContents(File file) throws IOException {

In the interests of avoiding problems across backouts, it might be worth splitting these safe changes into a Part 0.

::: mobile/android/search/java/org/mozilla/search/providers/SearchEngineManager.java
@@ +46,3 @@
>  
>      private Context context;
> +    private Distribution distribution;  

Nit: trailing WS.

@@ +132,5 @@
> +                    // First, look for the default search engine in a distribution.
> +                    name = getDefaultEngineNameFromDistribution();
> +                    if (name == null) {
> +                        // Otherwise, get the default engine that we ship.
> +                        name = getDefaultEngineNameFromLocale();   

Nit: trailing WS.

@@ +137,4 @@
>                      }
> +
> +                    // Store the default engine name for the future.
> +                    // XXX: This causes our onPreferenceChange listner to fire, which is annoying.

Either unregister then re-register, or increment an 'ignore' counter that the listener decrements.

@@ +145,4 @@
>                  }
>  
> +                final SearchEngine engine = createEngineFromName(name);
> +                runCallback(engine, callback);

cEFN can return null. Does the callback expect that?

@@ +151,4 @@
>      }
>  
>      /**
> +     * Looks for a default search engine inlcuded in a distribution.

included

@@ +158,2 @@
>       */
> +    private String getDefaultEngineNameFromDistribution() {

You need a check right at the start of this method:

  if (!this.distribution.exists()) {
      return null;
  }

@@ +215,5 @@
> +    /**
> +     * Creates a SearchEngine instance from an engine name.
> +     *
> +     * To create the engine, we first try to find the search plugin in the distribution
> +     * (if one exists), followed by the localized plugins we ship we the browser, and

we ship with

@@ +256,5 @@
> +     * @param name Search engine name.
> +     * @return SearchEngine instance for name.
> +     */
> +    private SearchEngine createEngineFromDistribution(String name) {
> +        final File pluginsDir = distribution.getDistributionFile("searchplugins");

Again, guard the start of this method with an exists() check.

@@ +283,5 @@
> +     *
> +     * This method reads the list of search plugin file names from list.txt, then
> +     * iterates through the files, creating SearchEngine instances until it finds one
> +     * with the right name. Unfortunately, we need to do this because there is no
> +     * other way to name the search engine "name" to the file for the search plugin.

way to map

@@ +301,5 @@
> +                final InputStream pluginIn = getInputStreamFromSearchPluginsJar(identifier + ".xml");
> +                final SearchEngine engine = createEngineFromInputStream(identifier, pluginIn);
> +                if (engine != null && engine.getName().equals(name)) {
> +                    return engine;
> +                }

These four lines are repeated in 342-345. Lift those out?

@@ +353,4 @@
>  
> +
> +    private SearchEngine createEngineFromInputStream(InputStream in) {
> +        return createEngineFromInputStream(null, in);

I'd be inclined to inline this method.

@@ +358,5 @@
> +
> +    /**
> +     * Creates a SearchEngine instance from an InputStream.
> +     *
> +     * This method closes the stream after it is done reading it. 

Nit: trailing WS.

Comment 21

[:Margaret Leibovic]

(In reply to Richard Newman [:rnewman] from comment #20)

> @@ +137,4 @@
> >                      }
> > +
> > +                    // Store the default engine name for the future.
> > +                    // XXX: This causes our onPreferenceChange listner to fire, which is annoying.
> 
> Either unregister then re-register, or increment an 'ignore' counter that
> the listener decrements.

I'll do the ignore counter idea. I was going to do an ignore boolean flag, but apply() is asynchronous, and I was worried that flipping the flag immediately after calling this wouldn't have the desired effect.

> @@ +145,4 @@
> >                  }
> >  
> > +                final SearchEngine engine = createEngineFromName(name);
> > +                runCallback(engine, callback);
> 
> cEFN can return null. Does the callback expect that?

Yes, I updated the callback consumers to deal with that (I decided it should be up to them to handle this, rather than just never getting their callback called), but I'll update the docs to make this more explicit.

> @@ +301,5 @@
> > +                final InputStream pluginIn = getInputStreamFromSearchPluginsJar(identifier + ".xml");
> > +                final SearchEngine engine = createEngineFromInputStream(identifier, pluginIn);
> > +                if (engine != null && engine.getName().equals(name)) {
> > +                    return engine;
> > +                }
> 
> These four lines are repeated in 342-345. Lift those out?

I think it would make more sense to lift out the logic to iterate through a file list to find a matching name. Maybe that's what you meant to suggest? I'm just going to do that :)

Comment 22

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/f9856f8ee44f
https://hg.mozilla.org/integration/fx-team/rev/8f4f59d06ef1

Tested this with:

1) Shipped default search engine (e.g. Yahoo!)
2) Default set to another engine we ship (e.g. Bing)
3) Default set to a third-party installed engine
4) Default set to a distribution search engine

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f9856f8ee44f
https://hg.mozilla.org/mozilla-central/rev/8f4f59d06ef1

Comment 24

[:Margaret Leibovic]

Attachment: Folded patch for aurora

Approval Request Comment
[Feature/regressing bug #]: search activity
[User impact if declined]: search activity will not use correct default search engine
[Describe test coverage new/current, TBPL]: no tests, landed on m-c, tested locally
[Risks and why]: medium risk, because this changes a lot of the logic for finding the default search engine in the search activity. However, overall it makes this logic easier to follow and more correct, and this is most definitely a necessary patch for us to ship the search activity.
[String/UUID change made/needed]: none

Note: We need to uplift bug 1093220 for this patch to apply cleanly.

Comment 25

[:Margaret Leibovic]

Attachment: Folded patch for aurora

Oops, some extraneous merge conflict changes ended up in there.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/70749d38fa9e