we need add l10n support for the onboarding overlay system addon

I use same approach as formAutofill to load the string bundle into onboarding.js

Comment on attachment 8873776 [details] Bug 1369291 - add l10n support for onboarding overlay; https://reviewboard.mozilla.org/r/145194/#review149172 Looks good to me.

Comment on attachment 8873776 [details] Bug 1369291 - add l10n support for onboarding overlay; https://reviewboard.mozilla.org/r/145194/#review149280

thanks!

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/6b6a52c561b2 add l10n support for onboarding overlay;r=mossop,rexboy

Backed out for packaging failures. https://hg.mozilla.org/integration/autoland/rev/869ba165ad1e01626267f4ca8fb5b46a2d66e0a3 https://treeherder.mozilla.org/logviewer.html#?job_id=104405488&repo=autoland

looks like need to add the locale path into http://searchfox.org/mozilla-central/source/browser/locales/Makefile.in#102

try green, land again

flod, we'd plan to ship onboarding overlay in 56. According to MDN pontoon page we seems need to 1. create a separate locale project in github or somewhere, and 2. sync periodically back to mozilla-central * https://developer.mozilla.org/en-US/docs/Mozilla/Implementing_Pontoon_in_a_Mozilla_website Do you have some suggest place to host the locale repo? (I think https://github.com//mozilla/onboarding-l10n is good according to https://github.com/mozilla/activity-stream-l10n) Do you know who has the permission to create the repo in github/mozilla and add us as admin for that repo? * Fred @gasolin, * rex @rexboy7, * fischer @Fischer.json

(In reply to Fred Lin [:gasolin] from comment #13) > flod, we'd plan to ship onboarding overlay in 56. Do you need/plan to ship string updates out of trains? In other words: if you land strings in mozilla-central while 56 is there, and not later (when it's in Beta), you don't necessarily need to move strings outside of Mercurial. Moving out of m-c means that you need to solve the infrastructure problem: ensuring that all locales ship a complete localization (all strings, no errors). We are also working on using one single repository to ship all versions of Firefox (cross-channel), we are not there yet but that might an important bit to consider if the timeline is for 56. > Do you have some suggest place to host the locale repo? > (I think https://github.com//mozilla/onboarding-l10n is good according to > https://github.com/mozilla/activity-stream-l10n) > > Do you know who has the permission to create the repo in github/mozilla and > add us as admin for that repo? > > * Fred @gasolin, > * rex @rexboy7, > * fischer @Fischer.json I cannot create repositories in the Mozilla organization, but I can in mozilla-l10n. That would also allow localizers to write strings. Before that, we need to figure out the first part though, i.e. if you really need to move out of m-c https://github.com/mozilla-l10n/

Also: why the 56 timeline when Photon is going to ship in 57?

I don't think we need anything pontoony here. You want to follow what https://hg.mozilla.org/mozilla-central/rev/8d9a2e5cbc2c does, aka, add to l10n.ini and filter.py. The documentation for that is on http://gecko.readthedocs.io/en/latest/python/compare-locales/index.html.

Thanks for clarify, I think we can live within mozilla-central before 57

(In reply to Fred Lin [:gasolin] from comment #17) > Thanks for clarify, I think we can live within mozilla-central before 57 I don't think we'll need to move out of mozilla-central even after, especially with cross-channel (and we should have it before 57). As to land these strings before they're actually used, that's OK, as long as we can provide UI mock-ups and instructions to localizers, so they don't localize without context.

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/22b83f0ffd51 add l10n support for onboarding overlay;r=mossop,rexboy

Comment on attachment 8873776 [details] Bug 1369291 - add l10n support for onboarding overlay; https://reviewboard.mozilla.org/r/145194/#review149714 Denoting an r- based on my comment 16. We need a fix to l10n.ini and filter.py still.

Backout by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/f3e352d6353e Backed out changeset 22b83f0ffd51 at Pike's request because it needs more work.

This had test failures too: https://treeherder.mozilla.org/logviewer.html#?job_id=104559757&repo=autoland

Localization should live in locales/en-US, not locale/en-US, and paths updated accordingly. Also jar.mn needs updating, compare for example with https://hg.mozilla.org/mozilla-central/rev/8d9a2e5cbc2c

Comment on attachment 8873776 [details] Bug 1369291 - add l10n support for onboarding overlay; https://reviewboard.mozilla.org/r/145194/#review150028 Found three more problems which I'd like to get changes for. FWIW, I don't have a good idea about the chrome urls not used test failure myself. ::: browser/extensions/onboarding/content/onboarding.js:15 (Diff revisions 4 - 10) > Cu.import("resource://gre/modules/Services.jsm"); > > const ONBOARDING_CSS_URL = "resource://onboarding/onboarding.css"; > const ABOUT_HOME_URL = "about:home"; > const ABOUT_NEWTAB_URL = "about:newtab"; > -const BUNDLE_URI = "chrome://onboarding/locale/onboarding.properties"; > +const BUNDLE_URI = "chrome://onboarding/locales/onboarding.properties"; The chrome url needs to stay /locale/, despite the fact that the src dir is locales. ::: browser/extensions/onboarding/content/onboarding.js:16 (Diff revisions 4 - 10) > > const ONBOARDING_CSS_URL = "resource://onboarding/onboarding.css"; > const ABOUT_HOME_URL = "about:home"; > const ABOUT_NEWTAB_URL = "about:newtab"; > -const BUNDLE_URI = "chrome://onboarding/locale/onboarding.properties"; > +const BUNDLE_URI = "chrome://onboarding/locales/onboarding.properties"; > +const BRAND_SHORT_NAME = Services.strings I think we should get this bundle on demand, too. ::: browser/extensions/onboarding/content/onboarding.js:78 (Diff revisions 4 - 10) > // The security should be fine because this is not from an external input. > // We're not shipping yet so l10n strings is going to be closed for now. > div.innerHTML = ` > <div id="onboarding-overlay-dialog"> > <button id="onboarding-tour-close-btn">X</button> > - <header>${this.bundle.GetStringFromName("gettingStarted")}</header> > + <header>${this._bundle.GetStringFromName("onboarding.overlay-title", [BRAND_SHORT_NAME], 1)}</header> Please don't put localized content into innerHTML, can you set the textContent of the header after creating the DOM? Localizations are only level 1 content, and this has security implications.

Fred, as I'm looking at this stuff, should this content be ifndef RELEASE_OR_BETA? Aka, does this feature ride the trains already?

Thanks for review. Yes it should flagged by ifndef RELEASE_OR_BETA!

local test of browser/base/content/test/static/browser_all_files_referenced.js passed, please kindly review again The extension itself is flagged by ifndef RELEASE_OR_BETA in browser/locales/Makefile.in , do I need some extra setting to let onboarding/locale get excluded from the train?

Comment on attachment 8873776 [details] Bug 1369291 - add l10n support for onboarding overlay; https://reviewboard.mozilla.org/r/145194/#review150090 Sorry, right intuition, but wrong detail. The onboarding extension is actually behind NIGHTLY_BUILD, and we'll need to use that in the Makefile, too. Not sure if there's actually still a difference between those two with devedition now being beta builds, but I also don't know which flags we pass there. Anywho, those two checks should be the same thing. That's the last thing I found, next round should be an r+. ::: browser/locales/Makefile.in:108 (Diff revision 11) > endif > @$(MAKE) -C ../extensions/pocket/locale AB_CD=$* XPI_NAME=locale-$* > ifndef RELEASE_OR_BETA > @$(MAKE) -C ../extensions/presentation/locale AB_CD=$* XPI_NAME=locale-$* > @$(MAKE) -C ../extensions/webcompat-reporter/locales AB_CD=$* XPI_NAME=locale-$* > + @$(MAKE) -C ../extensions/onboarding/locales AB_CD=$* XPI_NAME=locale-$* Actually, this needs to be ifdef NIGHTLY_BUILD to match the flags in https://dxr.mozilla.org/mozilla-central/source/browser/extensions/moz.build#32

Comment on attachment 8873776 [details] Bug 1369291 - add l10n support for onboarding overlay; https://reviewboard.mozilla.org/r/145194/#review151208 Thanks, looking good now. r=me, and I'll also trigger to land this on autoland.

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. hg error in cmd: hg rebase -s 2aec5d612b10 -d f1a9cda28b1b: rebasing 400734:2aec5d612b10 "Bug 1369291 - add l10n support for onboarding overlay;r=mossop,Pike,rexboy" (tip) merging browser/extensions/onboarding/content/onboarding.js warning: conflicts while merging browser/extensions/onboarding/content/onboarding.js! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)

NI on gasolin for the rebase.

Thanks! This bug is conflicted with bug 1369282 (which just checked in) (the above line of close button is changed)

Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/2cf913cce0c4 add l10n support for onboarding overlay;r=mossop,Pike,rexboy

How can I verify this fix for QA purposes?

Justin, best to pick the language of your choice, download nightly, and verify that it has the string as listed on https://dxr.mozilla.org/l10n-central/search?q=onboarding.overlay-title&redirect=true.

Perfect. Thanks Axel! I have verified this is fixed using the fr locale.