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

Status: RESOLVED FIXED

(Android Background Services Graveyard :: Core, defect, P3)

Description

[Richard Newman [:rnewman]]

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.

Comment 1

[Richard Newman [:rnewman]]

Nominally on my plate.

Comment 3

[Richard Newman [:rnewman]]

Attachment: Part 0: remove AppConstants dependency on .App.

This allows us to continue importing AppConstants into android-sync.

Comment 4

[Nick Alexander :nalexander [he/him]]

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?

Comment 5

[Richard Newman [:rnewman]]

Attachment: Part 0: remove AppConstants dependency on .App by moving things to TestConstants.

TestConstants version...

Comment 6

[Nick Alexander :nalexander [he/him]]

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.

Comment 7

[Richard Newman [:rnewman]]

Attachment: Part 1: rework LocaleManager to simplify use from android-sync. v1

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.

Comment 8

[Richard Newman [:rnewman]]

Attachment: Android Sync parts.

Not yet tested, but looks like it'll go smoothly.

Depends on the LocaleManager changes in Part 1.

Comment 9

[Richard Newman [:rnewman]]

Attachment: Part 0: remove AppConstants dependency on .App by moving things to TestConstants. v2

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?

Comment 10

[Richard Newman [:rnewman]]

Comment on attachment 8401865 [details] [diff] [review]
Part 0: remove AppConstants dependency on .App by moving things to TestConstants. v2

Carry-forward r+.

Comment 11

[Nick Alexander :nalexander [he/him]]

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)?

Comment 12

[Nick Alexander :nalexander [he/him]]

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.

Comment 13

[Richard Newman [:rnewman]]

(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.

Comment 14

[Richard Newman [:rnewman]]

Nick: outstanding question for you in Comment 9.

Comment 15

[Richard Newman [:rnewman]]

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.

Comment 16

[Richard Newman [:rnewman]]

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

Comment 17

[Nick Alexander :nalexander [he/him]]

Per discussion with rnewman, we just replaced the reflection in the particular case he cared about it.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/22935e9eb273

Comment 19

[Richard Newman [:rnewman]]

   https://hg.mozilla.org/integration/fx-team/rev/466700354958
   https://hg.mozilla.org/integration/fx-team/rev/661f96cbf4ce

Comment 20

[Richard Newman [:rnewman]]

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.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/466700354958
https://hg.mozilla.org/mozilla-central/rev/661f96cbf4ce