The default bug view has changed. See this FAQ.

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.