My idea from bug 928347: reviewers may forget to check l10n definition in properties, but this will introduce many localization missing bugs. However, it's easy to have a script point out these misses, can we add this check to Travis? (God bless those poor Travis machines.)

Hey Evelyn, I wrote a tiny framework for adding jobs to travis CI (jobs run in parallel so we can just add machines to keep travis times lowish) See https://github.com/mozilla-b2g/gaia/tree/master/tests/travis_ci Each one of those folders maps to an environment variable as seen here: https://github.com/mozilla-b2g/gaia/blob/master/.travis.yml#L15 So you could add an environment variable like "CI_ACTION=l10n_lint" and then add a l10n_lint folder with a script file which would do whatever checking is needed (see other folders for examples).

James, you really do a lot of cool things...

(In reply to James Lal [:lightsofapollo] from comment #1) > Hey Evelyn, > > I wrote a tiny framework for adding jobs to travis CI (jobs run in parallel > so we can just add machines to keep travis times lowish) > > See https://github.com/mozilla-b2g/gaia/tree/master/tests/travis_ci > > Each one of those folders maps to an environment variable as seen here: > > https://github.com/mozilla-b2g/gaia/blob/master/.travis.yml#L15 > > So you could add an environment variable like "CI_ACTION=l10n_lint" and then > add a l10n_lint folder with a script file which would do whatever checking > is needed (see other folders for examples). That's cool! I will find some time to try it. :)

Evelyn, this is a great idea and I agree that it would be immensely helpful. There are two things that are easy to check on build time: 1. we localize the visible DOM to all included locales, so we can track errors reported by l10n.js: /apps/bluetooth/transfer.html: [l10n] [en-US]: 1 missing in the visible DOM: exit 2. we can compare other languages' resources to the en-US.properties file to check how complete localizations are: /apps/browser/index.html: [l10n] [ar]: 2 missing in the visible DOM: top-sites-startPage, browserBrandShortName /apps/browser/index.html: [l10n] [ar]: 1 broken in the visible DOM: browser /apps/browser/index.html: [l10n] [ar]: 16 missing compared to en-US: brandShortName, browserBrandShortName, browserBrandFullName, top-sites-startPage, top-sites-tab, search-engines, default-search-engine, save-image, image-saved, error-saving-image, save-video, video-saved, error-saving-video, save-audio, audio-saved, error-saving-audio Currently, l10n.js does #1. We're planning to improve this and add #2 in bug 914414, but without making the tree go red. AFAICT, your idea goes beyond this. This is when it starts to be a bit more complex :) It's harded to the completness check for en-US because there is no absolute list of l10n-ids. (I guess this is what this bug is about) How would we create such list of all required l10n-ids to compare the en-US.properties file againt? Should we parse all html files, regardless of whether they're linked from index.html or not? Also, I don't think there's a reliable way to detect l10n-ids used in JavaScript, especially if they're composed programmatically (e.g. _('error-' + err.name) etc).

Bug 915059 - Remove unused l10n resource from b2g in gecko is related to this

Here is my proposal, after quick discussion with Yuren and Evelyn: Let limit the scope of this bug to only verify the l10n IDs in HTML (and HTML only) exists in en-US locale file. Since we pre-localize the HTML files in production build, if we replace the "missing property" warning there to a error throw, the build will fail and the "Gb" test on Gaia-Try will fail; therefore, we will catch the mistake before merging. Stas, does that work? What do you think?

Thanks, Tim, for flagging this. I created a WIP pull request here: https://github.com/mozilla-b2g/gaia/pull/23767 I wasn't sure what the output of failed tests would look like, but it turns out it's pretty good: https://tbpl.mozilla.org/?rev=c7fc4c2b87a3&tree=Gaia-Try There a few strings which currently throw (some are actual missing strings, others—I'm yet to understand why they throw). I'll work on this early next week.

Woot! If you are taking the bug do assign it to yourself :)

Gladly :)

Update: I was able to find and fix all cases (at least I hope so) where this approach was throwing errors for en-US. The reason why it used to throw was that in those cases the translation contained a placeable (e.g. {{name}}) which was only available on runtime. The fix was to remove the data-l10n-id attribute from the corresponding HTML nodes; the translated contents are inserted into the DOM by the runtime JS logic anyways. I still need to review the JS logic; I think we should be using mozL10n.setAttributes instead of node.textContent = 'translated value', because the former is faster and also automatically works when the language change, while the latter doesn't. I also need to write tests. I'll do this next week since I have other priorities for this week. The pull request is: https://github.com/mozilla-b2g/gaia/pull/23767 The most recent TBPL run is: https://tbpl.mozilla.org/?rev=77d79fcd25c4&tree=Gaia-Try

I rebased my branch on top of the fix from bug 1048411 which simplified the code a little bit. I still need to add tests for this. https://tbpl.mozilla.org/?rev=fe0b97f3c95d&tree=Gaia-Try

New WIP PR: https://github.com/mozilla-b2g/gaia/pull/25309

This was low on my list but I finally found time to make a lot of progress. I now have a fully green build with integration tests working. https://github.com/mozilla-b2g/gaia/pull/25309 https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=6ccdf9888d08 The functional part of the patch was the easy part. Fixing bugs in existing apps and writing new tests was a bit more challenging :) Asking Gandalf for the general review and Ricky for the build part. Ricky, I moved build integration tests for l10n to a separate file because I anticipate a few more tests coming soon (bug 1013831, bug 1091113).

Comment on attachment 8526745 [details] [diff] [review] Patch 1 Review of attachment 8526745 [details] [diff] [review]: ----------------------------------------------------------------- Once I started to throw in the build system when missing strings were discovered, a number of small bugs surfaced which would break the build. I fixed all of them and made changes across all of Gaia, so I'd like to ask for feedback from people related to the modules that I touched. Also, Flod, can you take a look at the changed and added string here and make sure they look sane to you, please? ::: apps/callscreen/index.html @@ +23,5 @@ > <link rel="localization" href="../communications/dialer/locales/shared.{locale}.properties"> > <link rel="localization" href="shared/locales/date/date.{locale}.properties"> > <link rel="localization" href="shared/locales/phone_types/phone_types.{locale}.properties"> > <link rel="localization" href="shared/locales/lockscreen/lockscreen.{locale}.properties"> > + <link rel="localization" href="shared/locales/keypad/keypad.{locale}.properties"> drs: I had to add this file here to fix errors about missing entities in callscreen: L10nError: "phoneNumberInput" not found in en-US in app://callscreen.gaiamobile.org ::: apps/collection/locales/collection.en-US.properties @@ +13,5 @@ > edit-done=Done > > # delete-collection > +delete-title2=Delete Collection > +delete-body2=The Collection and all of its data will be deleted. Kevin: I wasn't able to find where the {{name}} var was passed to this string. The HTML used to have the string embedded and it said 'Collection' instead of {{name}}. If there's some JS logic that uses mozL10n.setAttributes to pass the name of the collection into this string then I think we can safely remove data-l10n-ids from the HTML altogether. ::: apps/communications/dialer/locales/dialer.en-US.properties @@ +27,5 @@ > createNewContact=Create new contact > delete=Delete > deselectAll=Deselect all > dialerAddContact.ariaLabel=Add contact > +dialerAddContactDialog.ariaLabel=Add Contact Dialog drs: I also added two missing strings in dialer.en-US.properties. Here… @@ +71,5 @@ > from-withheld-number=From withheld number > yesterday=Yesterday > sending=Sending… > speakerOption.ariaLabel=Speaker > +onHoldOption.ariaLabel=Put on hold …and here. ::: apps/ftu/index.html @@ -136,5 @@ > </gaia-header> > <article role="main"> > <section id="pincode-screen" role="region"> > <label id="pin-label"></label> > - <label id="pin-retries-left" data-l10n-id="inputCodeRetriesLeft" class="retries hidden">Unknown tries left</label> Sam: there's code in apps/ftu/js/sim_manager.js which uses mozL10n.setAttributes to set the data-l10n-id and args for this elements and the other ones in this file. I think it's safe to remove the data-l10n-id here. ::: apps/system/locales/system.en-US.properties @@ -275,5 @@ > error-title={{name}} is having problems > -airplane-is-on=Airplane mode is on > -airplane-is-turned-on={{name}} requires a network connection. Turn off airplane mode from Settings and connect to a Wi-Fi or mobile data network. > -network-connection-unavailable=Network connection unavailable > -network-error={{name}} requires a network connection. Try connecting to a Wi-Fi or mobile data network. Alive: these strings seem to be not used anywhere so I removed them. ::: build/config/phone/apps-engineering.list @@ -32,5 @@ > dev_apps/contacts-manager > dev_apps/contacts-ds-provider1 > dev_apps/contacts-ds-provider2 > dev_apps/demo-hci-event > -dev_apps/test-l10n-optimize I removed this app from the list because I modified the tests to set the app list dynamically just for the test. ::: shared/pages/import/curtain.html @@ +52,5 @@ > </section> > > <!-- Error card --> > <section id="error"> > + <h1 id="errorMsg">Error</h1> Kevin: same as above, I think it's safe to remove the data-l10n-id here since it's set dynamically by the JS code anyways.

Comment on attachment 8526745 [details] [diff] [review] Patch 1 Review of attachment 8526745 [details] [diff] [review]: ----------------------------------------------------------------- Patch almost no problem for me. Please fix a nit and set r=@RickyChien thanks! ::: build/webapp-optimize.js @@ +122,5 @@ > // change the language of the localization context > mozL10n.ctx.requestLocales(this.locales[processedLocales]); > > // create JSON dicts for the current language for locales-obj/ > + if (this.config.GAIA_CONCAT_LOCALES) { this.config.GAIA_CONCAT_LOCALES === '1'

Comment on attachment 8526745 [details] [diff] [review] Patch 1 Review of attachment 8526745 [details] [diff] [review]: ----------------------------------------------------------------- In general it looks good to me. ::: apps/collection/locales/collection.en-US.properties @@ +13,5 @@ > edit-done=Done > > # delete-collection > +delete-title2=Delete Collection > +delete-body2=The Collection and all of its data will be deleted. If we need to drop the {{name}}, I think that "This collection and all of its data will be deleted" works better. But I think this is called from apps/homescreen/js/request.js https://github.com/mozilla-b2g/gaia/blob/master/apps/homescreen/js/request.js#L117-L118 ::: apps/communications/dialer/locales/dialer.en-US.properties @@ +71,5 @@ > from-withheld-number=From withheld number > yesterday=Yesterday > sending=Sending… > speakerOption.ariaLabel=Speaker > +onHoldOption.ariaLabel=Put on hold This just landed from bug 1069836

(In reply to Francesco Lodolo [:flod] from comment #16) > Comment on attachment 8526745 [details] [diff] [review] > Patch 1 > > Review of attachment 8526745 [details] [diff] [review]: > ----------------------------------------------------------------- > > In general it looks good to me. > > ::: apps/collection/locales/collection.en-US.properties > @@ +13,5 @@ > > edit-done=Done > > > > # delete-collection > > +delete-title2=Delete Collection > > +delete-body2=The Collection and all of its data will be deleted. > > If we need to drop the {{name}}, I think that "This collection and all of > its data will be deleted" works better. > > But I think this is called from apps/homescreen/js/request.js > https://github.com/mozilla-b2g/gaia/blob/master/apps/homescreen/js/request. > js#L117-L118 Ah, I see. So it looks like we might need two versions of this string: a generic one, to put in the HTML, and a specific one, calling out the name of the collection. Kevin, what do you think? > > ::: apps/communications/dialer/locales/dialer.en-US.properties > @@ +71,5 @@ > > from-withheld-number=From withheld number > > yesterday=Yesterday > > sending=Sending… > > speakerOption.ariaLabel=Speaker > > +onHoldOption.ariaLabel=Put on hold > > This just landed from bug 1069836 Nice, thanks! I'll rebase after the first round of reviews.

(In reply to Staś Małolepszy :stas from comment #17) > Ah, I see. So it looks like we might need two versions of this string: a > generic one, to put in the HTML, and a specific one, calling out the name of > the collection. Kevin, what do you think? Actually, it looks like this code uses its own confirm dialog (not gaia-confirm): https://github.com/mozilla-b2g/gaia/blob/master/apps/homescreen/elements/confirm_dialog.html and its own strings: https://github.com/mozilla-b2g/gaia/blob/master/apps/homescreen/locales/homescreen.en-US.properties#L8-L12

Comment on attachment 8526745 [details] [diff] [review] Patch 1 Review of attachment 8526745 [details] [diff] [review]: ----------------------------------------------------------------- FTU party looks good. ::: apps/ftu/index.html @@ +136,5 @@ > </gaia-header> > <article role="main"> > <section id="pincode-screen" role="region"> > <label id="pin-label"></label> > + <label id="pin-retries-left" class="retries hidden">Unknown tries left</label> Lets go ahead and remove this placeholder string, as we dont have a translation for it and the user should never see it. @@ +148,5 @@ > </section> > > <section id="pukcode-screen" role="region"> > <label id="puk-label"></label> > + <label id="puk-retries-left" class="retries hidden">Unknown tries left</label> Same here, kill the 'Unknown tries left'

Comment on attachment 8526745 [details] [diff] [review] Patch 1 Review of attachment 8526745 [details] [diff] [review]: ----------------------------------------------------------------- ::: apps/collection/locales/collection.en-US.properties @@ +13,5 @@ > edit-done=Done > > # delete-collection > +delete-title2=Delete Collection > +delete-body2=The Collection and all of its data will be deleted. Yeah - that seems correct, though I don't know the old home screen too well anymore. Not sure if I can give feedback+ if we change this because I don't think it's intentional?

Comment on attachment 8526745 [details] [diff] [review] Patch 1 Review of attachment 8526745 [details] [diff] [review]: ----------------------------------------------------------------- f+ for system part

(In reply to Kevin Grandon :kgrandon (In Europe/Conf until 11/24) from comment #20) > ::: apps/collection/locales/collection.en-US.properties > @@ +13,5 @@ > > edit-done=Done > > > > # delete-collection > > +delete-title2=Delete Collection > > +delete-body2=The Collection and all of its data will be deleted. > > Yeah - that seems correct, though I don't know the old home screen too well > anymore. Not sure if I can give feedback+ if we change this because I don't > think it's intentional? Not sure if I follow, sorry. What isn't intentional?

(In reply to Staś Małolepszy :stas from comment #22) > Not sure if I follow, sorry. What isn't intentional? It seems like we're still passing the name as an argument at the call site, but the string no longer has it? Unless I'm missing something.

(In reply to Kevin Grandon :kgrandon (In Europe/Conf until 11/24) from comment #23) > It seems like we're still passing the name as an argument at the call site, > but the string no longer has it? Unless I'm missing something. So I wasn't able to find where the {{name}} var was passed to this string. In comment 16 flod suggested that 'name' came from https://github.com/mozilla-b2g/gaia/blob/master/apps/homescreen/js/request.js#L117-L118 but in comment 18 I discovered this code used its own strings. I still don't know if there's any code that passes the 'name' argument to apps/collection/delete.html's delete-body string. Do you have any pointers?

(In reply to Staś Małolepszy :stas from comment #24) > So I wasn't able to find where the {{name}} var was passed to this string. > In comment 16 flod suggested that 'name' came from > https://github.com/mozilla-b2g/gaia/blob/master/apps/homescreen/js/request. > js#L117-L118 but in comment 18 I discovered this code used its own strings. > I still don't know if there's any code that passes the 'name' argument to > apps/collection/delete.html's delete-body string. Do you have any pointers? Oh right, I think I remember now. This file was used at one point, and now it's mainly there for third party home screens and is not currently being used. I think the change makes sense after all, thanks!

Comment on attachment 8526745 [details] [diff] [review] Patch 1 Review of attachment 8526745 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, the Dialer part looks good! ::: apps/communications/dialer/locales/dialer.en-US.properties @@ +71,5 @@ > from-withheld-number=From withheld number > yesterday=Yesterday > sending=Sending… > speakerOption.ariaLabel=Speaker > +onHoldOption.ariaLabel=Put on hold Yeah, you'll have to rebase and remove this.

Comment on attachment 8526745 [details] [diff] [review] Patch 1 Review of attachment 8526745 [details] [diff] [review]: ----------------------------------------------------------------- ::: build/l10n.js @@ +35,5 @@ > if (DEBUG) { > + ctx.addEventListener('fetcherror', error); > + ctx.addEventListener('manifesterror', warn); > + ctx.addEventListener('parseerror', warn); > + ctx.addEventListener('notfounderror', error); Wouldn't it make more sense to send addBuildMessage from inside of the previous event listener for this error instead of adding another event listener? Minor, I'll accept it either way.

Thanks everyone for quick reviews and feedback! Merged: https://github.com/mozilla-b2g/gaia/commit/ba9e22dafdf079f039a741b879bbdf25bce16597 L20n.js: https://github.com/l20n/l20n.js/commit/35536c52e4c0f5e8754522575363cfb3154edbae