Closed Bug 1300735 Opened 3 years ago Closed 3 years ago

l10n data doesn't update on extension update, requires browser restart

Categories

(Add-on SDK Graveyard :: General, defect)

defect
Not set

Tracking

(firefox51 fixed)

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: gaubugzilla, Assigned: gaubugzilla)

References

Details

Attachments

(1 file, 1 obsolete file)

Steps to reproduce:

1. Add a file locale/en-US.properties to your extension with the following line:

  foo=bar

Also, put the following statement into lib/main.js:

  console.error("Value of foo: " + require("sdk/l10n").get("foo"));

2. Install extension. Press Ctrl/Cmd-Shift-J to open Browser Console - note the message "Value of foo: bar".
3. Now change the contents of locale/en-US.properties to the following:

  foo=not bar

4. Install extension. Press Ctrl/Cmd-Shift-J to open Browser Console.

Expected results:

The new message in Browser console says: "Value of foo: not bar"

Actual results:

The message says: "Value of foo: bar". Only after restarting the browser you get the correct value displayed.

This is because of nsIStringBundleService caching string bundles. Add-on SDK needs to call nsIStringBundleService.flushBundles() when starting up, this will make sure that any strings are reloaded.
Assignee: nobody → trev.moz
Status: NEW → ASSIGNED
Blocks: sdk/l10n
Attached patch Proposed patch (obsolete) — Splinter Review
Attachment #8788583 - Flags: review?(lgreco)
Comment on attachment 8788583 [details] [diff] [review]
Proposed patch

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

This patch looks good, I'm not particularly used to this part of the SDK, but looking at how the `flushBundles` helper is used internally (currently it seems to be used only in the Fennec internals) the change makes sense.

I was initially a bit unsure about the module choosen to apply the change, but after looking into the 'sdk/l10n' directory, I agree that the one choosen seems to be the better one.

I'm going to set the f+ flag on this patch, and I'm converting the patch into an hg patch that can be applied unmodified to mozilla-central and I'm going to push it to try (given the change the jetpack testsuite should not behave differently from how it behaves on the current version of the SDK, but it is always better to check it on try before landing a patch, than discovering it from a backout message).

I'm not a Peer of the SDK, and so I cannot set the final r+ on the this patch, but I'm going to set the r? flag assigned to Matteo (:zer0) on the patch converted to hg (and submitted to try) that I'm going to attach, then Matteo is going to completing the review on it.
Attachment #8788583 - Flags: review?(lgreco) → feedback+
Comment on attachment 8788583 [details] [diff] [review]
Proposed patch

Marking obsolete.

This attachment has been converted to a hg patch (Attachment 8788935 [details]) which can be directly applied to a mozilla-central repo (with the author metadata preserved).
Attachment #8788583 - Attachment is obsolete: true
Comment on attachment 8788935 [details]
Bug 1300735 - Flush cached locale strings when l10n SDK module is loaded.

Re-new the f+ flag on the patch converted to hg.
Attachment #8788935 - Flags: feedback+
Try build, restricted to the mochitest-jetpack test suite, related to this change (attachment 8788935 [details]):

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f8c3a9622e3
Comment on attachment 8788935 [details]
Bug 1300735 - Flush cached locale strings when l10n SDK module is loaded.

https://reviewboard.mozilla.org/r/77254/#review76230

Looks good to me!
Attachment #8788935 - Flags: review?(zer0) → review+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/142fd07295b0
Flush cached locale strings when l10n SDK module is loaded. r=zer0
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/142fd07295b0
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.