Re-implement MozLoopService localization code

RESOLVED FIXED in Firefox 35

Status

defect
P2
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mikedeboer, Assigned: mikedeboer, Mentored)

Tracking

unspecified
mozilla36
Points:
1
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-moztrap -
qe-verify -

Firefox Tracking Flags

(firefox35 fixed, firefox36 fixed)

Details

(Whiteboard: [tech-debt])

Attachments

(1 attachment)

- The localizedStrings property of MozLoopServiceInternal is implemented around the outdated assumption that the locale keys may contains dots ('.') to discern various types of strings, thus populates the cache with objects unnecessarily.
 - The getStrings() method of MozLoopService therefore returns objects, instead of strings which its name tends to suggest, and even serializes them to JSON.

This is all needlessly complicated; there are no locale key names with dots in loop.properties, so we can just cache a map of strings. If we were to introduce a key with dots, the current implementation would yield confusing results.
Flags: qe-verify-
Flags: firefox-backlog+
The reason we do it this way is to be compatible with content/libs/l10n.js which expects this format - we use l10n.js as a wrapper between chrome & content; to work and provide a similar interface to the gaia one that standalone uses.

We haven't used the dot format in loop.properties as we're not using the l10n.js code to translate elements - as we're filling the strings in dynamically.

If we can keep some of the existing interface to the content code, and simplify the backend, I'm all for that.
OS: Mac OS X → All
Hardware: x86 → All
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Flags: needinfo?(mmucci)
Attachment #8526028 - Flags: review?(nperriault)
Iteration: --- → 36.3
Points: --- → 1
backlog: --- → Fx36+
Priority: -- → P2
Added to IT 36.3
Flags: needinfo?(mmucci)
Blocks: 1074667
Comment on attachment 8526028 [details] [diff] [review]
Patch v1: simplify l10n code in MozLoopService

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

Looks much better! Ship it :)

::: browser/components/loop/MozLoopService.jsm
@@ +681,5 @@
>    /**
>     * A getter to obtain and store the strings for loop. This is structured
>     * for use by l10n.js.
>     *
> +   * @returns {Map} a map of element ids with localized string values

Nit: Missing full stop at line end.

@@ -709,3 @@
>      }
>  
> -    return gLocalizedStrings = map;

Wow. Thanks for removing this.
Attachment #8526028 - Flags: review?(nperriault) → review+
https://hg.mozilla.org/mozilla-central/rev/2bb6548450f7
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Comment on attachment 8526028 [details] [diff] [review]
Patch v1: simplify l10n code in MozLoopService

Approval Request Comment
[Risks and why]: Loop rooms code needed for 35 pref-on.  Avoids massive annoyance uplifting any other patches
Low risk, simplifies things
[String/UUID change made/needed]: none
Attachment #8526028 - Flags: approval-mozilla-aurora?
Attachment #8526028 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: in-moztrap-
You need to log in before you can comment on or make changes to this bug.