1131438 - SearchEngineManager doesn't scan the distro searchplugins locale folders

Status: RESOLVED FIXED

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

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: distro-search-locale-files v0.1

We have code to look in searchplugins/common, but not searchplugins/locale/<locale-code>

This patch adds it. Note that the SearchEngineManager uses the Java Locale which is not exactly the same way Gecko does it.

Comment 1

[Richard Newman [:rnewman]]

Comment on attachment 8561873 [details] [diff] [review]
distro-search-locale-files v0.1

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

Keeping the f? on Margaret 'cos I want her thoughts, too!

::: mobile/android/search/java/org/mozilla/search/providers/SearchEngineManager.java
@@ +457,5 @@
> +        if (localeDir != null) {
> +            final String languageTag = Locales.getLanguageTag(Locale.getDefault());
> +            final File[] localeFiles = (new File(localeDir, languageTag)).listFiles();
> +            if (localeFiles != null) {
> +                Collections.addAll(files, localeFiles);

Rather than collecting two File[]s into an ArrayList, then making a File[] out of it, I suggest doing:

  if (localeDir != null) {
      ...
      SearchEngine e = (files == null) ? null : createEngineFromFileList(files, name);
      if (e != null) {
          return e;
      }
  }

  // Fall back to the common folder.
  file File[] commonFiles = …
  

That way you don't allocate anything extra, and you don't do the fallback work if the locale override yields an engine.

@@ +467,5 @@
> +        if (commonFiles != null) {
> +            Collections.addAll(files, commonFiles);
> +        }
> +
> +        if (files.size() == 0) {

files.isEmpty()

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Richard Newman [:rnewman] from comment #1)
> Comment on attachment 8561873 [details] [diff] [review]
> distro-search-locale-files v0.1
> 
> Review of attachment 8561873 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Keeping the f? on Margaret 'cos I want her thoughts, too!
> 
> :::
> mobile/android/search/java/org/mozilla/search/providers/SearchEngineManager.
> java
> @@ +457,5 @@
> > +        if (localeDir != null) {
> > +            final String languageTag = Locales.getLanguageTag(Locale.getDefault());
> > +            final File[] localeFiles = (new File(localeDir, languageTag)).listFiles();
> > +            if (localeFiles != null) {
> > +                Collections.addAll(files, localeFiles);
> 
> Rather than collecting two File[]s into an ArrayList, then making a File[]
> out of it, I suggest doing:
> 
>   if (localeDir != null) {
>       ...
>       SearchEngine e = (files == null) ? null :
> createEngineFromFileList(files, name);
>       if (e != null) {
>           return e;
>       }
>   }
> 
>   // Fall back to the common folder.
>   file File[] commonFiles = …
>   
> 
> That way you don't allocate anything extra, and you don't do the fallback
> work if the locale override yields an engine.

I used this approach first, but I ended up not liking the way the code flowed. I found the collection of files to be easier to read and a better code flow.

I didn't care about the extra allocation. It's quite small.

Comment 3

[:Margaret Leibovic]

Comment on attachment 8561873 [details] [diff] [review]
distro-search-locale-files v0.1

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

Looks fine to me, but I'm worried there are probably edge cases all over the place here. Like, a distribution can include a different version of a search plugin that's included with the browser. Or it can include locale specific plugins for some locales but also include common plugins.

In the long term, it would be nice if we could write up a matrix of different situations we support and add a test to testDistribution. I wonder if testDistribution could import SearchEngineManager and make a call to getEngine() to check the result.

::: mobile/android/search/java/org/mozilla/search/providers/SearchEngineManager.java
@@ +451,5 @@
>  
> +        // Collect an array of files to scan
> +        ArrayList<File> files = new ArrayList<>();
> +
> +        // First, check to see if there's a locale-specific override.

Add a comment to explain this mirrors appendDistroSearchDirs in DirectoryProvider.

@@ +464,5 @@
> +
> +        // Fallback to the common folder
> +        final File[] commonFiles = (new File(pluginsDir, "common")).listFiles();
> +        if (commonFiles != null) {
> +            Collections.addAll(files, commonFiles);

Should we actually do this bit first? Looking at appendDistroSearchDirs, we add the common files before adding the locale-specific ones:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/DirectoryProvider.js#103

Do we also need to worry about this default locale business?
http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/DirectoryProvider.js#143

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to :Margaret Leibovic from comment #3)
> Comment on attachment 8561873 [details] [diff] [review]
> distro-search-locale-files v0.1
> 
> Review of attachment 8561873 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks fine to me, but I'm worried there are probably edge cases all over the
> place here. Like, a distribution can include a different version of a search
> plugin that's included with the browser. Or it can include locale specific
> plugins for some locales but also include common plugins.

A distribution:
1. can include a different version of a search plugin that's included with the browser
2. can include locale specific plugins for some locales but also include common plugins

Indeed, and we should be able to handle these now. #1 was working before, but #2 needs this patch.

> In the long term, it would be nice if we could write up a matrix of
> different situations we support and add a test to testDistribution. I wonder
> if testDistribution could import SearchEngineManager and make a call to
> getEngine() to check the result.

I'll look into this in a new bug,

> > +        // First, check to see if there's a locale-specific override.
> 
> Add a comment to explain this mirrors appendDistroSearchDirs in
> DirectoryProvider.

Done

> > +        // Fallback to the common folder
> > +        final File[] commonFiles = (new File(pluginsDir, "common")).listFiles();
> > +        if (commonFiles != null) {
> > +            Collections.addAll(files, commonFiles);
> 
> Should we actually do this bit first? Looking at appendDistroSearchDirs, we
> add the common files before adding the locale-specific ones:
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/
> DirectoryProvider.js#103

Yes. Done.

> Do we also need to worry about this default locale business?
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/
> DirectoryProvider.js#143

Looks like we should. I added simple support for it.

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: distro-search-locale-files v0.2

* Moved "common" file check to be first
* Added code to pull the distributionLocale from preferences.json and made the comment match the fallbackLocale: Should only be used on background thread, but mainly for sync issues, not performance issues.
* Added code to look at distributionLocale if we didn't find any plugins in the expected locale folder.

Comment 6

[:Margaret Leibovic]

Comment on attachment 8562992 [details] [diff] [review]
distro-search-locale-files v0.2

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

Looks good. It would be nice to get a wiki page or something to document all the different factors that go into finding a user's default search engine.

::: mobile/android/search/java/org/mozilla/search/providers/SearchEngineManager.java
@@ +248,5 @@
>          try {
>              final JSONObject all = new JSONObject(FileUtils.getFileContents(prefFile));
>  
> +            // First, look for a default locale specified by the distribution.
> +            if (all.has("Preferences")) {

This makes me think we're getting close to re-implementing the JS distribution code in Java as well :/

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

https://hg.mozilla.org/integration/fx-team/rev/d680979a2f99

Comment 8

[Carsten Book [:Tomcat]]

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