Closed Bug 1057631 Opened 6 years ago Closed 6 years ago

Use real locale when pulling search engine out of the jar

Categories

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

All
Android
defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 35

People

(Reporter: Margaret, Assigned: Margaret)

References

Details

Attachments

(1 file, 1 obsolete file)

We'll want to fix bug 1057629 before we do this.
Priority: -- → P1
Background: we're hacking together a path to pull search plugins out of the jar, and right now we hard-code it to use the en-US path:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/search/java/org/mozilla/search/providers/SearchEngineManager.java#160

I'm not sure of the best way to go about fixing this bug. My first instinct is to just use the system locale, but I realized that if we're using strings from Fennec for the search activity (bug 1024527), the search activity may be in a gecko locale that's different from the system's.

rnewman, where do we store the current locale for Fennec? Is there a SharedPreference I can read?
Flags: needinfo?(rnewman)
So long as all of our application entry points do the right BrowserLocaleManager dance, you can just use Locale.getDefault(): in the course of applying the user's preference, we set Locale.

(You should be doing that BrowserLocaleManager dance in the search activity, too. See <https://ci.mozilla.org/job/mozilla-central-docs/Tree_Documentation/fennec/localeswitching.html>.)

getAndApplyPersistedLocale -- which those docs suggest you should use -- actually returns the locale code for you, so you can hold onto it when you leave the dance floor and start looking for a search plugin.
Flags: needinfo?(rnewman)
First of all, I'm not sure about this string replace to swap the underscore for the dash. Is there any place in the tree where we already have to handle something like this?

Secondly, I'm not sure about the fallback to look for just the language. How do we currently handle the case where the system has a region-specific locale, but we only support a language (e.g. fr_FR and fr).
Attachment #8483830 - Flags: feedback?(rnewman)
(In reply to :Margaret Leibovic from comment #3)

> First of all, I'm not sure about this string replace to swap the underscore
> for the dash. Is there any place in the tree where we already have to handle
> something like this?

BrowserLocaleManager.getLanguageTag will do the right thing for a given Locale object.


> Secondly, I'm not sure about the fallback to look for just the language. How
> do we currently handle the case where the system has a region-specific
> locale, but we only support a language (e.g. fr_FR and fr).

Various ad hoc methods. E.g., see chrome/nsChromeRegistryChrome.cpp:54, which does; versus browser/components/dirprovider/DirectoryProvider.cpp, which doesn't fall back.

Desktop doesn't really have this problem: you install a Firefox in a language you picked, and you can't change it. On Android we're in a slightly different situation, because the OS locale might be a sibling or child of a supported locale (es-US => es-ES, fr-CA => fr). A fallback is a good idea in that case.

(For strings we fall back, but we don't fall sideways: Bug 933315.)
Status: NEW → ASSIGNED
Comment on attachment 8483830 [details] [diff] [review]
WIP - Use real locale when pulling search engine out of the jar

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

::: mobile/android/search/java/org/mozilla/search/providers/SearchEngineManager.java
@@ +219,5 @@
> +    private InputStream getInputStreamFromJar(String fileName) {
> +        final Locale locale = Locale.getDefault();
> +
> +        // First, try a file path for the full locale.
> +        String url = getSearchPluginsJarUrl(locale.toString().replace("_", "-"), fileName);

You can use BLM.getLanguageTag(locale) here.

@@ +227,5 @@
> +            return in;
> +        }
> +
> +        // If that doesn't work, try a file path for just the language.
> +        url = getSearchPluginsJarUrl(locale.getLanguage(), fileName);

You probably want to check if localeTag.equals(language), and don't try twice for `fr` and friends.

Bear in mind that Java gets language tags wrong; see the translation code in getLanguageTag.

I encourage you to add a getLanguage(locale) method to BLM, which would do only the first part of getLanguageTag(locale).

@@ +234,5 @@
>  
>      /**
>       * Gets the jar URL for a file in the searchplugins directory
>       *
> +     * @param locale String representing the gecko locale (e.g. "en-US")

Nit: s/gecko/Gecko, throughout.

@@ +241,2 @@
>       */
> +    private String getSearchPluginsJarUrl(String locale, String fileName) {

Nit: s/Url/URL, throughout.
Attachment #8483830 - Flags: feedback?(rnewman) → feedback+
This appears to be working.

Corresponding PR: https://github.com/mozilla/fennec-search/pull/80

I just had to add a stub implementation for getLanguageTag. It doesn't really matter what it does, since the standalone activity will never successfully get anything out of the jar directory (that's why we have a fallback to read search plugins from assets).
Attachment #8483830 - Attachment is obsolete: true
Attachment #8484573 - Flags: review?(rnewman)
Depends on: 1063274
Comment on attachment 8484573 [details] [diff] [review]
Use real locale when pulling search engine out of the jar

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

r+ with this change.

::: mobile/android/search/java/org/mozilla/search/providers/SearchEngineManager.java
@@ +229,5 @@
> +            return in;
> +        }
> +
> +        // If that doesn't work, try a file path for just the language.
> +        final String language = locale.getLanguage();

= BrowserLocaleManager.getLanguage(locale);
Attachment #8484573 - Flags: review?(rnewman) → review+
https://hg.mozilla.org/mozilla-central/rev/f33e9c8cd993
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.