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

RESOLVED FIXED in Firefox 31

Status

Android Background Services
Core
P3
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: rnewman, Assigned: rnewman)

Tracking

(Depends on: 1 bug, 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)

(Assignee)

Description

3 years ago
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.
(Assignee)

Updated

3 years ago
Blocks: 957381
(Assignee)

Updated

3 years ago
tracking-fennec: --- → 31+
Priority: -- → P3
(Assignee)

Comment 1

3 years ago
Nominally on my plate.
Assignee: nobody → rnewman
Duplicate of this bug: 957381
(Assignee)

Comment 3

3 years ago
Created attachment 8401028 [details] [diff] [review]
Part 0: remove AppConstants dependency on .App.

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+
(Assignee)

Comment 5

3 years ago
Created attachment 8401070 [details] [diff] [review]
Part 0: remove AppConstants dependency on .App by moving things to TestConstants.

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+
(Assignee)

Comment 7

3 years ago
Created attachment 8401078 [details] [diff] [review]
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.
Attachment #8401078 - Flags: review?(nalexander)
(Assignee)

Updated

3 years ago
Attachment #8401028 - Attachment is obsolete: true
(Assignee)

Comment 8

3 years ago
Created attachment 8401079 [details] [review]
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)
(Assignee)

Updated

3 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 9

3 years ago
Created attachment 8401865 [details] [diff] [review]
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?
(Assignee)

Updated

3 years ago
Attachment #8401070 - Attachment is obsolete: true
(Assignee)

Comment 10

3 years ago
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+
(Assignee)

Comment 13

3 years ago
(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.
(Assignee)

Comment 14

3 years ago
Nick: outstanding question for you in Comment 9.
Flags: needinfo?(nalexander)
(Assignee)

Comment 15

3 years ago
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.
(Assignee)

Comment 16

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/22935e9eb273
Whiteboard: [leave open]
Per discussion with rnewman, we just replaced the reflection in the particular case he cared about it.
Flags: needinfo?(nalexander)
https://hg.mozilla.org/mozilla-central/rev/22935e9eb273
Flags: in-testsuite+
(Assignee)

Comment 19

3 years ago
   https://hg.mozilla.org/integration/fx-team/rev/466700354958
   https://hg.mozilla.org/integration/fx-team/rev/661f96cbf4ce
Whiteboard: [leave open] → [qa+]
Target Milestone: --- → Firefox 31
(Assignee)

Comment 20

3 years ago
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.
https://hg.mozilla.org/mozilla-central/rev/466700354958
https://hg.mozilla.org/mozilla-central/rev/661f96cbf4ce
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.