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

Status: VERIFIED FIXED

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

Description

[Aaron Train [:aaronmt]]

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

Comment 1

[:Margaret Leibovic]

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

Comment 2

[Richard Newman [: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!

Comment 3

[Richard Newman [:rnewman]]

Attachment: Flush browser.js Strings cache on locale change. v1

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.

Comment 4

[Richard Newman [:rnewman]]

Attachment: Flush browser.js Strings cache on locale change. v2

Tested and working.

Comment 5

[Richard Newman [:rnewman]]

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.

Comment 6

[: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.

Comment 7

[:Margaret Leibovic]

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.

Comment 8

[:Margaret Leibovic]

Generalizing this bug.

Comment 9

[: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]:
-----------------------------------------------------------------

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.

Comment 10

[Richard Newman [:rnewman]]

Margaret: do you think we should WONTFIX this for 32?

Comment 11

[Richard Newman [:rnewman]]

Attachment: Flush browser.js Strings cache on locale change. (minimal)

This alone seems to work. Yay!

Comment 12

[: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.

Comment 13

[Richard Newman [:rnewman]]

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

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d94b93409b31

Comment 15

[Richard Newman [:rnewman]]

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.

Comment 16

[Richard Newman [:rnewman]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/aa013c7e579b