String was introduced in bug 972992 feedback_window_will_close_in=This window will close in {{countdown}} seconds This is broken for English when countdown is 1, but it's a lot more broken for other locales with complex plural forms. WebL10n has support for plural forms, not sure about the version used by Loop.

IMHO, loop should update to the gaia v2.0 branch version.

Where do you see the warning being displayed? I do not see it anywhere. We're not actually using the main webl10n library here, this is in desktop, so we are using something similar to what pdfjs uses to access the chrome: http://mxr.mozilla.org/mozilla-central/source/browser/components/loop/content/libs/l10n.js To fix this, we probably need to expose PluralForm.get (from gecko's PluralForm.jsm) onto MozLoopAPI and then find a way to expose it to l10n.js.

I made an assumption: if you added the string, it's going to be displayed somewhere, right? AFAIK the build I'm using doesn't have those strings yet.

(In reply to Francesco Lodolo [:flod] from comment #3) > I made an assumption: if you added the string, it's going to be displayed > somewhere, right? Ah, I misunderstood the title, I thought it meant there was an actual warning displayed about the missing plural form. > AFAIK the build I'm using doesn't have those strings yet. The latest nightly build should now have them.

How about just adding 2 different string ids? eg. feedback_window_will_close_in=This window will close in {{countdown}} second feedback_window_will_close_in_plural=This window will close in {{countdown}} seconds So we could just: var stringId = "feedback_window_will_close_in" + (countdown > 0 ? "_plural" : ""); Just a cent.

(In reply to Nicolas Perriault (:NiKo`) from comment #6) > How about just adding 2 different string ids? eg. Suggested reading https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_and_Plurals P.S. that code fails even for French...

Okay, I was just trying to find something s{hort|impl}er. So here's my concerns with the proposed approach if we want to go that path: - it feels odd to expose getPluralForm to the mozLoop API; if we really want to expose l10n related stuff through it, we should directly expose the full translation API and use it from desktop content… but: - standalone app doesn't have access to the mozLoop object (which is desktop only); so we're introducing yet another difference for l10n management between the two environments. We should rather try to focus on sharing the same tools rather than increasing segmentation here, imho - we're touching l10n.js directly, while this is already a duplicate from the version used for pdf.js; I think forking it entirely may increase maintenance pain over time; at least we should try to patch the original file (or file a bug about it anyway if this lands); I could totally see an abstraction on top of two different adapters for l10n, so at least we could consume a single API from both content envs. - The patch misses testing.

Yeah, using l10n.js, sob. If you're updating to anything, use l10n.js from the gaia v2.0 branch or master. I don't think we should use that for gecko code, tbh. standalone app, is that https://github.com/mozilla-b2g/firefoxos-loop-client/ ? If so, its localization shouldn't bother us here, as we're doing that in that github repo, rather separate from this code, AFAICT.

Axel the standalone app is not the firefoxos-loop-client one, it is the one extracted from m-c/browser/components/loop/standalone here https://github.com/mozilla/loop-client

(In reply to Axel Hecht [:Pike] from comment #9) > Yeah, using l10n.js, sob. If you're updating to anything, use l10n.js from > the gaia v2.0 branch or master. > I don't think we should use that for gecko code, tbh. If you have insights or advice for how to properly deal with l10n in Loop, please comment on bug 1000269. > standalone app, is that > https://github.com/mozilla-b2g/firefoxos-loop-client/ ? Nope, it's the static pages served to call link clickers; the code is part of the gecko tree, in browser/components/loop/standalone, and shares a bunch of assets with Loop desktop content code.

I added a test that covers MozLoopAPI.jsm but not the functionality in l10n.js. Let me know if that is not enough.

To clarify a couple of points: - We can't currently use gaia's l10n.js in the desktop loop directly. There's issues with the XHRs as they are trying to load chrome files from content (due to the separation that we've got set up). Its conceivable in future, we might be able to work around that in some way, but this isn't the bug to resolve that. - I would be tempted to use the plural format that gaia uses, as it means that the L10n file would handle that. However, this would mean a different format (to the rest of gecko) for localisers to handle. Additionally, its unclear whether through the use of properties files, if the gaia l10n.js would support the "gecko-style" plural format. I've just filed bug 1059776 to investigate these in the next cycle. For now, I think we should take this patch and let that investigation work out if there's something better that we can do.

I hate to be a pain but changes to existing strings need new IDs https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_best_practices#Changing_existing_strings Sorry for not spotting this earlier, I realized only while checking the strings landed on m-c.

Thanks for catching my mistake flod.

Comment on attachment 8481393 [details] [diff] [review] follow-up to change string ID Great, thanks to both of you.

Untracking for QE since I don't think this is something we need to verify. I believe this will basically be covered through smoketesting locales. Please needinfo me to request more specific testing.