Closed Bug 1020502 Opened 5 years ago Closed 5 years ago

String bundles from .properties files don't reflect locale changes until browser restart

Categories

(Firefox for Android :: Locale switching and selection, defect)

32 Branch
All
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 33
Tracking Status
firefox31 --- unaffected
firefox32 --- fixed
firefox33 --- verified
fennec 32+ ---

People

(Reporter: aaronmt, Assigned: rnewman)

References

Details

Attachments

(2 files, 1 obsolete file)

Another sibling of bug 997589?

Change browser language, invoke a toast (close about:home), see previous language used in the super-toast button (e.g, undo).
(In reply to Aaron Train [:aaronmt] from comment #0)
> Another sibling of bug 997589?

No, this would have a different cause. We pull the strings for the undo close tab toast from the browser.properties string bundle:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#986

rnewman, how does updating string bundles like that on locale change work?
Flags: needinfo?(rnewman)
We should invalidate the Strings cache:

  http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#254

when the locale change propagates to Gecko. This'll be a similar hook to its sibling bugs.

Good spot, Aaron!
Flags: needinfo?(rnewman)
Hardware: ARM → All
tracking-fennec: --- → ?
tracking-fennec: ? → 32+
Assignee: nobody → rnewman
A more elegant fix for this would be to watch the locale pref itself, which would be more complete and less coupled. I might do that for Bug 997589. But this is shorter.

Haven't tested yet, but worth reviewing.
Attachment #8437434 - Flags: review?(margaret.leibovic)
Tested and working.
Attachment #8437453 - Flags: review?(mark.finkle)
Attachment #8437434 - Attachment is obsolete: true
Attachment #8437434 - Flags: review?(margaret.leibovic)
Comment on attachment 8437453 [details] [diff] [review]
Flush browser.js Strings cache on locale change. v2

Wait, that was supposed to go to margaret again.
Attachment #8437453 - Flags: review?(mark.finkle) → review?(margaret.leibovic)
Comment on attachment 8437453 [details] [diff] [review]
Flush browser.js Strings cache on locale change. v2

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

r+, but I would review a new version if you choose to make one :)

::: mobile/android/chrome/content/browser.js
@@ +253,5 @@
> +
> +/**
> + * Return an object mapping identifiers to lazy getters for string bundles.
> + */
> +function getLazyStrings() {

My one concern is that there is already a ton of stuff in the global scope here, so I don't love adding even more things, even if they're unlikely to collide with stuff.

Maybe we could wrap this as part of the Strings object? Just a brainstorm, what about something like this:

var Strings = {
  PROPERTIES_FILES: [..],

  invalidate: function () {
    for (let s of this.PROPERTIES_FILES) {
      let [name, bundle] = s;
      delete this.name; // not sure if we need this, not sure how defineLazyGetter is implemented

      XPCOMUtils.defineLazyGetter(this, name, function () {
        return Services.strings.createBundle(bundle);
      }
    }
  }
}
Strings.invalidate();

Too magical? This would break if we ever includes an "invalidate" string bundle, but I think we can be smart enough to avoid that.
Attachment #8437453 - Flags: review?(margaret.leibovic) → review+
Also, I wonder if we should move this Strings stuff into a JSM, since we're going to run into this issue all over the app wherever we create string bundles:

http://mxr.mozilla.org/mozilla-central/search?find=/mobile/android/&string=Services.strings.createBundle

Hell, we create a bundle for browser.properties in a bunch of places. If we moved this to some shared module, the bundles could all be lazy-loaded/invalidated in one place.
Generalizing this bug.
Summary: Super-toast notifications don't reflect locale changes until browser restart → String bundles from .properties files don't reflect locale changes until browser restart
Comment on attachment 8437453 [details] [diff] [review]
Flush browser.js Strings cache on locale change. v2

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

Sorry for all the churn! I think we should try to fix this a better way that will fix the issue everywhere, rather than fixing a few instances, which will cover up the root problem.
Attachment #8437453 - Flags: review+ → review-
Margaret: do you think we should WONTFIX this for 32?
This alone seems to work. Yay!
Attachment #8437745 - Flags: review?(margaret.leibovic)
Comment on attachment 8437745 [details] [diff] [review]
Flush browser.js Strings cache on locale change. (minimal)

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

Excellent.
Attachment #8437745 - Flags: review?(margaret.leibovic) → review+
https://hg.mozilla.org/integration/fx-team/rev/d94b93409b31
Status: NEW → ASSIGNED
Flags: needinfo?(rnewman)
https://hg.mozilla.org/mozilla-central/rev/d94b93409b31
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
Status: RESOLVED → VERIFIED
Comment on attachment 8437745 [details] [diff] [review]
Flush browser.js Strings cache on locale change. (minimal)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
  Bug 917480.

User impact if declined: 
  Toasts and other JS-driven UI elements can show strings in the previous locale after a locale switch.

Testing completed (on m-c, etc.): 
  Baked on Nightly.

Risk to taking this patch (and alternatives if risky): 
  Should be totally safe.

String or IDL/UUID changes made by this patch:
  None.
Attachment #8437745 - Flags: approval-mozilla-aurora?
Flags: needinfo?(rnewman)
Attachment #8437745 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Blocks: 1096120
You need to log in before you can comment on or make changes to this bug.