Invoke LocaleManager locale switching code prior to handling strings or Locale in background services

RESOLVED FIXED in Firefox 31

Status

defect
P3
normal
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: rnewman, Assigned: rnewman)

Tracking

(Blocks 1 bug)

Firefox 30
Firefox 31
All
Android
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(fennec31+)

Details

(Whiteboard: [qa+])

Attachments

(3 attachments, 2 obsolete attachments)

We can't rely on GeckoApp having been launched and thus locales to have been processed. Whenever we do work with strings -- presenting the FxA activities, popping up a notification from a sent tab, or just calling Locale.getDefault() -- we need to prompt the LocaleManager to do its work.

This blocks the deployment of locale switching UI to GA.
Blocks: 957381
tracking-fennec: --- → 31+
Priority: -- → P3
Nominally on my plate.
Assignee: nobody → rnewman
Duplicate of this bug: 957381
This allows us to continue importing AppConstants into android-sync.
Attachment #8401028 - Flags: review?(nalexander)
Comment on attachment 8401028 [details] [diff] [review]
Part 0: remove AppConstants dependency on .App.

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

This basically rolls back Bug 990116, which I just landed, so I'm not thrilled; but I agree that AppConstants should be treated like BrowserContract.  I wish we enforced that!

We still have TestConstants.java.in, so you could move BROWSER_INTENT_CLASS there, and have it not use reflection; I have a gentle preference for that.  (And remove the existing duplicated package name, which should just reference AppConstants.)  What say you?
Attachment #8401028 - Flags: review?(nalexander) → feedback+
TestConstants version...
Attachment #8401070 - Flags: review?(nalexander)
Comment on attachment 8401070 [details] [diff] [review]
Part 0: remove AppConstants dependency on .App by moving things to TestConstants.

lgtm.  Sorry to make you churn.
Attachment #8401070 - Flags: review?(nalexander) → review+
This patch does two things:

* It splits out the Fennec-touching parts of LocaleManager into a separate interface/class pair. This allows us to stub out those parts in android-sync.

* It reduces the necessary surface area of LocaleManager by implicitly initing if necessary.
Attachment #8401078 - Flags: review?(nalexander)
Attachment #8401028 - Attachment is obsolete: true
Posted file Android Sync parts.
Not yet tested, but looks like it'll go smoothly.

Depends on the LocaleManager changes in Part 1.
Attachment #8401079 - Flags: review?(nalexander)
Status: NEW → ASSIGNED
Nick: I noticed that BrowserInstrumentationTests *also* had a dependency on BROWSER_INTENT_CLASS. I doubt this is landable, though, because BIT can't see TestConstants (apart from in Eclipse).

Could you either suggest a solution, or implement it and land the patch on fx-team for me?
Attachment #8401070 - Attachment is obsolete: true
Comment on attachment 8401865 [details] [diff] [review]
Part 0: remove AppConstants dependency on .App by moving things to TestConstants. v2

Carry-forward r+.
Attachment #8401865 - Flags: review+
Comment on attachment 8401078 [details] [diff] [review]
Part 1: rework LocaleManager to simplify use from android-sync. v1

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

::: mobile/android/base/BrowserLocaleManagerIntegration.java
@@ +10,5 @@
> +
> +import android.content.Context;
> +import android.content.SharedPreferences;
> +
> +public class BrowserLocaleManagerIntegration implements LocaleManagerIntegration {

Class comment!

@@ +20,5 @@
> +    }
> +
> +    @Override
> +    public SharedPreferences getSharedPreferences(Context context) {
> +        // TODO: this should be per-profile, but we don't want to pay the price.

Tracking ticket?

::: mobile/android/base/LocaleManager.java
@@ +42,5 @@
>      private static volatile boolean inited = false;
>      private static boolean systemLocaleDidChange = false;
>      private static BroadcastReceiver receiver;
>  
> +    // This is a horrible hack, but it keeps us moving.

Explain why?  Because there's strange name spacing with the *Integration class being stubbed differently by the two different consumers (Fennec and ABS)?
Attachment #8401078 - Flags: review?(nalexander) → review+
Comment on attachment 8401079 [details] [review]
Android Sync parts.

Comments on the github PR.  r+, with reservations about whether the abstraction layer is correct, and whether the coupling between Fennec and ABS is correct.
Attachment #8401079 - Flags: review?(nalexander) → review+
(In reply to Nick Alexander :nalexander from comment #11)

> > +    public SharedPreferences getSharedPreferences(Context context) {
> > +        // TODO: this should be per-profile, but we don't want to pay the price.
> 
> Tracking ticket?

I'll fork Bug 949495 Comment 17 into a separate bug, but I think I killed this comment locally, because it's not something on which I wish to act any time soon, having identified two escape routes.
Nick: outstanding question for you in Comment 9.
Flags: needinfo?(nalexander)
I landed Part 0 using reflection in BrowserTestCase, because I want to keep moving forward and I don't know how to either (a) refer to Robocop's test classes from BrowserInstrumentationTests, or (b) correctly preprocess BrowserTestCase. We can fix that later if we care.
Per discussion with rnewman, we just replaced the reflection in the particular case he cared about it.
Flags: needinfo?(nalexander)
For the record: something I changed here was to always update the Resources configuration during a locale change. We regularly hit an instance where the locale was already correct (!), and thus we short-circuited. Apparently it's necessary to give the Resources instance a kick in the pants at the start of the activity, so we get the right strings when we load the view.
You need to log in before you can comment on or make changes to this bug.