Closed Bug 1131438 Opened 5 years ago Closed 5 years ago

SearchEngineManager doesn't scan the distro searchplugins locale folders

Categories

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

x86
macOS
defect
Not set

Tracking

(firefox38 fixed)

RESOLVED FIXED
Firefox 38
Tracking Status
firefox38 --- fixed

People

(Reporter: mfinkle, Assigned: mfinkle)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch distro-search-locale-files v0.1 (obsolete) — Splinter Review
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.
Attachment #8561873 - Flags: review?(rnewman)
Attachment #8561873 - Flags: feedback?(margaret.leibovic)
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()
Attachment #8561873 - Flags: review?(rnewman)
Attachment #8561873 - Flags: feedback?(margaret.leibovic)
Attachment #8561873 - Flags: feedback+
(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 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
Attachment #8561873 - Flags: feedback?(margaret.leibovic) → feedback+
(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.
* 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.
Assignee: nobody → mark.finkle
Attachment #8561873 - Attachment is obsolete: true
Attachment #8562992 - Flags: review?(margaret.leibovic)
Depends on: 1132250
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 :/
Attachment #8562992 - Flags: review?(margaret.leibovic) → review+
https://hg.mozilla.org/mozilla-central/rev/d680979a2f99
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.