Counterpart to bug 1451992 is to migrate Preferences Subdialogs off of .properties to Fluent.

This is a fairly early WIP. It migrates all subdialogs off of .properties except of permissions.xul/js, which is tricky because it uses <tree> XBL widget.

Can you take a look a this patchset? I think it's code-wise complete and ready for migration scripts but would like to get your feedback before I start requesting reviews :) And if you have time to help me with the migration scripts, the floor is yours as well!

Comment on attachment 8971079 [details] Bug 1457021 - Migrate the JS of Preferences::SitePermissions to Fluent. https://reviewboard.mozilla.org/r/239854/#review248114 ::: browser/components/preferences/sitePermissions.js:132 (Diff revision 3) > break; > case Services.perms.DENY_ACTION: > - stringKey = "cannot"; > + stringKey = "permissions-capabilities-cannot"; > break; > case Services.perms.PROMPT_ACTION: > - stringKey = "prompt"; > + stringKey = "permissions-capabiltiies-prompt"; typo: capabilities ::: browser/locales/en-US/browser/preferences/permissions.ftl:53 (Diff revision 3) > > permissions-searchbox = > .placeholder = Search Website > + > + > +permissions-capabilities-can = permissions-capabilities-allow ::: browser/locales/en-US/browser/preferences/permissions.ftl:55 (Diff revision 3) > .placeholder = Search Website > + > + > +permissions-capabilities-can = > + .label = Allow > +permissions-capabilities-cannot = permissions-capabilities-block

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #16) > And if you have time to help me with the migration scripts, the floor is > yours as well! How do you feel about having one migration for the entire bug, instead of a ton of small migrations?

> How do you feel about having one migration for the entire bug, instead of a ton of small migrations? Totally fine! :) I would keep the patches separate for the code because they make it easier to review, but we can get one migration.

Comment on attachment 8971082 [details] Bug 1457021 - Migrate the JS of Preferences::SiteData to Fluent. https://reviewboard.mozilla.org/r/239860/#review248118 ::: browser/locales/en-US/browser/preferences/preferences.ftl:724 (Diff revision 2) > ## Privacy Section - Site Data > > sitedata-header = Cookies and Site Data > > +sitedata-total-size-calculating = Calculating site data and cache size… > +sitedata-total-size = Your stored cookies, site data and cache are currently using { $value } { $unit } of disk space. We should add a comment about the placeholders ::: browser/locales/en-US/browser/preferences/siteDataSettings.ftl:38 (Diff revision 2) > > site-data-button-save = > .label = Save Changes > .accesskey = a > > +site-usage-pattern = { $value } { $unit } Same here, I would add a quick note to explain what $value and $unit are, with an example. ::: browser/locales/en-US/browser/preferences/siteDataSettings.ftl:53 (Diff revision 2) > + > ## Removing > > -site-data-removing-window = > +site-data-removing-dialog = > .title = { site-data-removing-header } > + .buttonlabelaccept = Remove This attribute is a mouthful. Maybe just "acceptbutton"?

Comment on attachment 8971083 [details] Bug 1457021 - Migrate the JS of Preferences::Languages to Fluent. https://reviewboard.mozilla.org/r/239862/#review248120 ::: browser/locales/en-US/browser/preferences/languages.ftl:36 (Diff revision 2) > > languages-customize-add = > .label = Add > .accesskey = A > + > +languages-code-format = Add a comment about the parameters and an example. Should $locale be $language?

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #19) > > How do you feel about having one migration for the entire bug, instead of a ton of small migrations? > > Totally fine! :) > > I would keep the patches separate for the code because they make it easier > to review, but we can get one migration. OK. I gave a first pass to the patches, and they look good, only a few comments missing. I'll write the migration, hopefully before the end of the day, and double check the string content.

Comment on attachment 8971082 [details] Bug 1457021 - Migrate the JS of Preferences::SiteData to Fluent. https://reviewboard.mozilla.org/r/239860/#review248156 ::: browser/locales/en-US/browser/preferences/siteDataSettings.ftl:53 (Diff revision 2) > + > ## Removing > > -site-data-removing-window = > +site-data-removing-dialog = > .title = { site-data-removing-header } > + .buttonlabelaccept = Remove On second thoughts: "button accept" for a button that says "Remove" is confusing. Why do we set this as an attribute of the dialog? Can't we set it up as a standard button?

Comment on attachment 8971080 [details] Bug 1457021 - Migrate the JS of Preferences::Blocklists to Fluent. https://reviewboard.mozilla.org/r/239856/#review248166 ::: browser/locales/en-US/browser/preferences/blocklists.ftl:36 (Diff revision 2) > +blocklist-item-list-template = { $listName } { $description } > + > +blocklist-item-moz-std-name = Disconnect.me basic protection (Recommended). > +blocklist-item-moz-std-desc = Allows some trackers so websites function properly. > +blocklist-item-moz-full-name = Disconnect.me strict protection. > +blocklist-item-moz-full-desc = Blocks known trackers. Some sites may not function properly. Some **websites**, not **sites**

Comment on attachment 8971083 [details] Bug 1457021 - Migrate the JS of Preferences::Languages to Fluent. https://reviewboard.mozilla.org/r/239862/#review248170 ::: browser/locales/en-US/browser/preferences/languages.ftl:37 (Diff revision 2) > languages-customize-add = > .label = Add > .accesskey = A > + > +languages-code-format = > + .label = { $locale } [{ $code }] Note: the original string has a double space here. I think it's correct to only have one space, just noting to explain the diff in migrated content.

Note that I've used these string IDs: permissions-capabilities-allow permissions-capabilities-block And used 'removebutton' as attribute for site-data-removing-dialog. Kept $locale in languages-code-format

Comment on attachment 8971082 [details] Bug 1457021 - Migrate the JS of Preferences::SiteData to Fluent. https://reviewboard.mozilla.org/r/239860/#review248156 > On second thoughts: "button accept" for a button that says "Remove" is confusing. > > Why do we set this as an attribute of the dialog? Can't we set it up as a standard button? I don't want to refactor the whole XUL dialog, and that's the name that is part of XUL `<dialog>` API used everywhere: https://searchfox.org/mozilla-central/search?q=buttonlabelaccept&case=false&regexp=false&path= I'd prefer to leave it as is at this point. I understand that it's confusing but it is an label for a button that accepts the dialog. The dialog is about removal.

Comment on attachment 8972009 [details] Bug 1457021 - Fix a mangled message ID order in a Preferences::Permissions JS dialog. https://reviewboard.mozilla.org/r/240748/#review248432 ::: browser/components/preferences/permissions.js:133 (Diff revision 4) > } > } catch (ex) { > document.l10n.formatValues([ > ["permissions-invalid-uri-title"], > ["permissions-invalid-uri-label"] > - ]).then(([message, title]) => { > + ]).then(([title, message]) => { This is pretty stupid bug, but fortunately it only landed yesterday in 62, so no need to backport.

Comment on attachment 8971079 [details] Bug 1457021 - Migrate the JS of Preferences::SitePermissions to Fluent. https://reviewboard.mozilla.org/r/239854/#review248430 ::: browser/components/preferences/sitePermissions.js:384 (Diff revision 6) > break; > > case "statusCol": > sortFunc = (a, b) => { > - return a.capabilityString.localeCompare(b.capabilityString); > + return parseInt(a.querySelector("menulist").value) > > + parseInt(b.querySelector("menulist").value); This is an interesting tibid. Before that, the sorting was by the value of the selection, which in English happens to be "Allow"/"Always Ask"/"Block". But that requires everything to be synchronous which is totally fine except of tests where we set the new site permission and then immediatelly proceed to test the new value. Since the code reacts to an event in an observer, there's no easy way to block on it. So instead, I used the fact that localization doesn't have to block here and we can set the domain name and allow/always allow/deny and sort them using the value of the menuitem (1 - allow, 2- block, 3 - prompt). This by default sorts clustering by action type, but not necessarily alphabetically in the locale, which I think is fine. Alternative would require us to somehow notify when the UI is updated and block on that in tests, which I would love to avoid.

Comment on attachment 8971081 [details] Bug 1457021 - Migrate the JS of Preferences::Fonts to Fluent. https://reviewboard.mozilla.org/r/239858/#review248426 ::: browser/components/preferences/fonts.js:93 (Diff revision 4) > writeUseDocumentFonts() { > var useDocumentFonts = document.getElementById("useDocumentFonts"); > return useDocumentFonts.checked ? 1 : 0; > }, > > - onBeforeAccept() { > + async onBeforeAccept() { A non trivial change here is that I moved the warning confirm prompt from on accept of the dialog, to on selection of the item. My main reason was that in order for that to work in async manner, I cannot use `onbeforeaccept`, but quite honestly, I think it also makes sense from the UX standpoint. Instead of asking the user for confirmation once he finishes his settings in a disconnected fashion, we now verify with them immediatelly after their selection. Either way, I thought it's worth noting that this is the only functional change in the whole patchset.

Comment on attachment 8974204 [details] Bug 1457021 - Add l10n migration script for Preferences Subdialog migration to Fluent. https://reviewboard.mozilla.org/r/242496/#review248440 ::: python/l10n/fluent_migrations/bug_1457021_js_subdialogs.py:26 (Diff revision 3) > + .label = { COPY("browser/chrome/browser/preferences/preferences.properties", "can") } > +permissions-capabilities-block = > + .label = { COPY("browser/chrome/browser/preferences/preferences.properties", "cannot") } > +permissions-capabilities-prompt = > + .label = { COPY("browser/chrome/browser/preferences/preferences.properties", "prompt") } > +permissions-invalid-uri-title = { COPY("browser/chrome/browser/preferences/preferences.properties", "invalidURITitle") } permissions-invalid-uri-title and permissions-invalid-uri-label showed up in your patch, but are due to a rebase :-\ They should be removed from this migration

Comment on attachment 8971079 [details] Bug 1457021 - Migrate the JS of Preferences::SitePermissions to Fluent. https://reviewboard.mozilla.org/r/239854/#review248444

Comment on attachment 8971080 [details] Bug 1457021 - Migrate the JS of Preferences::Blocklists to Fluent. https://reviewboard.mozilla.org/r/239856/#review248446

Comment on attachment 8971081 [details] Bug 1457021 - Migrate the JS of Preferences::Fonts to Fluent. https://reviewboard.mozilla.org/r/239858/#review248448

Comment on attachment 8971082 [details] Bug 1457021 - Migrate the JS of Preferences::SiteData to Fluent. https://reviewboard.mozilla.org/r/239860/#review248450

Comment on attachment 8971083 [details] Bug 1457021 - Migrate the JS of Preferences::Languages to Fluent. https://reviewboard.mozilla.org/r/239862/#review248452

Comment on attachment 8971080 [details] Bug 1457021 - Migrate the JS of Preferences::Blocklists to Fluent. https://reviewboard.mozilla.org/r/239856/#review248512 ::: browser/components/preferences/blocklists.js:147 (Diff revision 4) > - } > - list.selected = this._getActiveList() === id; > + id, name, > + selected: this._getActiveList() === id One property per line, please, and trailing comma after the last property, too.

Comment on attachment 8971083 [details] Bug 1457021 - Migrate the JS of Preferences::Languages to Fluent. https://reviewboard.mozilla.org/r/239862/#review248514 ::: browser/components/preferences/languages.js:55 (Diff revision 5) > // ab = language > // cd = region > var bundleAccepted = document.getElementById("bundleAccepted"); > - var bundlePreferences = document.getElementById("bundlePreferences"); > > - function LanguageInfo(aName, aABCD, aIsVisible) { > + function LocaleInfo(aLocaleName, aLocaleCode, aIsVisible) { Now we just need to rename the rest of the things in the file... ;-) (Follow-up if you feel strongly, though to be fair the header is accept-language...) ::: browser/components/preferences/languages.js:117 (Diff revision 5) > + let comp = new Services.intl.Collator(undefined, { > + usage: "sort" > + }); For my edification, what's the difference between this and `localeCompare` ? ::: browser/components/preferences/languages.js:127 (Diff revision 5) > + items.forEach((item, idx) => { > + if (idx > 0) { > + frag.insertBefore(item, items[idx - 1]); > + } > + }); OK... this is head-achy. You've got an array of items, and for every item but the first, you're inserting it before the previous item (so in effect, reversing the list). So I assume you've deliberately inverted the sort in the previous `.sort()` operation? Am I misreading this? IMO it would be more straightforward if this were just: ```js let items = Array.from(frag.children); items.sort(...); // Re-append items in the correct order: items.forEach(item => frag.appendChild(item)); ``` Unless that's slower for some reason, like because the list is already mostly-sorted? I guess you could optimize to something like this: ```js items.forEach((item, idx) => { if (frag.children[idx] != item) { frag.insertBefore(item, frag.children[idx + 1]); } }); ``` but then please add a comment to describe what's going on. :-) ::: browser/locales/en-US/browser/preferences/languages.ftl:36 (Diff revision 5) > +# Variables: > +# $locale (String) - A name of the locale (for example: "Icelandic", "Spanish (Chile)") > +# $code (String) - Locale code of the locale (for example: "is", "es-CL") The original had an example complete string. Given the 3 different types of braces/brackets involved, I think this comment would benefit from one of those, too. :-)

Comment on attachment 8972009 [details] Bug 1457021 - Fix a mangled message ID order in a Preferences::Permissions JS dialog. https://reviewboard.mozilla.org/r/240748/#review248516 Oops.

Comment on attachment 8971084 [details] Bug 1457021 - Migrate the JS of Preferences::Translation to Fluent. https://reviewboard.mozilla.org/r/239864/#review248522 Huh.

Comment on attachment 8971082 [details] Bug 1457021 - Migrate the JS of Preferences::SiteData to Fluent. https://reviewboard.mozilla.org/r/239860/#review248584 This looks reasonable, thanks!

Comment on attachment 8971079 [details] Bug 1457021 - Migrate the JS of Preferences::SitePermissions to Fluent. https://reviewboard.mozilla.org/r/239854/#review248624

Comment on attachment 8971081 [details] Bug 1457021 - Migrate the JS of Preferences::Fonts to Fluent. https://reviewboard.mozilla.org/r/239858/#review248626 ::: browser/components/preferences/fonts.js:93 (Diff revision 4) > writeUseDocumentFonts() { > var useDocumentFonts = document.getElementById("useDocumentFonts"); > return useDocumentFonts.checked ? 1 : 0; > }, > > - onBeforeAccept() { > + async onBeforeAccept() { We should rename this function now that it doesn't get fired with onbeforeaccept event. Can you rename it to confirmMinSizeChange?

Comment on attachment 8971083 [details] Bug 1457021 - Migrate the JS of Preferences::Languages to Fluent. https://reviewboard.mozilla.org/r/239862/#review248514 > OK... this is head-achy. You've got an array of items, and for every item but the first, you're inserting it before the previous item (so in effect, reversing the list). So I assume you've deliberately inverted the sort in the previous `.sort()` operation? Am I misreading this? > > IMO it would be more straightforward if this were just: > > ```js > let items = Array.from(frag.children); > items.sort(...); > // Re-append items in the correct order: > items.forEach(item => frag.appendChild(item)); > ``` > > Unless that's slower for some reason, like because the list is already mostly-sorted? I guess you could optimize to something like this: > > ```js > items.forEach((item, idx) => { > if (frag.children[idx] != item) { > frag.insertBefore(item, frag.children[idx + 1]); > } > }); > ``` > > but then please add a comment to describe what's going on. :-) Thanks! That's a great cleanup. Fixing :)

> Now we just need to rename the rest of the things in the file... ;-) > (Follow-up if you feel strongly, though to be fair the header is accept-language...) Not necessarily. So, I know it's confusing, and I'm trying to push some documentation to help (bug 1438687 - gonna get to it real soon!), but the gist is: - language - "French", "Italian", "Polish" are languages - locale is a combination of language/region/script/regional-prefs - "Spanish (Chile)", "English (American), but with 24h clock and buddhist calendar" and now, here's the kicker: - language tag - a locale encoded as a BCP47 string - "es-CL" or "en-US-u-hc-h240-ca-buddhist" Yep... :( So, accept-languages header makes sense because it actually talks about language tags! While when you encode a language tag as an object you'll store a locale (see https://github.com/tc39/proposal-intl-locale/ which we'll hopefully soon have - bug 1433303). > For my edification, what's the difference between this and `localeCompare` ? Performance. Every Intl formatter has been designed to shift as much of the perf cost as possible into the constructor. Which means that you will have always two operations: - constructor (heavy) - executing function (light) If you execute the operation in the loop, it's faster to do the constructor ones. `String.localeCompare` and `Date.toLocaleString` etc. are all helper functions that internally call the constructor and then executing function. The complication here is that if you're trying to be very eager and overoptimize by trying to cache the intl formatter in some permanent storage, you lock down language selection and need to invalidate on language switch. In my experience the sweet spot is to use object methods most of the time, and separate out the constructor when called in a loop. HTH!

Comment on attachment 8971082 [details] Bug 1457021 - Migrate the JS of Preferences::SiteData to Fluent. carrying over r+

Pushed by zbraniecki@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ceab67aa97d4 Migrate the JS of Preferences::Blocklists to Fluent. r=flod,Gijs https://hg.mozilla.org/integration/autoland/rev/7bb95530ceeb Migrate the JS of Preferences::Fonts to Fluent. r=flod,jaws https://hg.mozilla.org/integration/autoland/rev/8f74f964537d Migrate the JS of Preferences::SiteData to Fluent. r=flod,johannh https://hg.mozilla.org/integration/autoland/rev/4332073c7382 Migrate the JS of Preferences::Languages to Fluent. r=flod,Gijs https://hg.mozilla.org/integration/autoland/rev/4d66223323b4 Migrate the JS of Preferences::Translation to Fluent. r=Gijs https://hg.mozilla.org/integration/autoland/rev/3beeaf20f46a Migrate the JS of Preferences::SitePermissions to Fluent. r=flod,jaws https://hg.mozilla.org/integration/autoland/rev/34bc72be5d28 Fix a mangled message ID order in a Preferences::Permissions JS dialog. r=Gijs https://hg.mozilla.org/integration/autoland/rev/af2f85201ec2 Add l10n migration script for Preferences Subdialog migration to Fluent. r=flod

https://hg.mozilla.org/mozilla-central/rev/ceab67aa97d4 https://hg.mozilla.org/mozilla-central/rev/7bb95530ceeb https://hg.mozilla.org/mozilla-central/rev/8f74f964537d https://hg.mozilla.org/mozilla-central/rev/4332073c7382 https://hg.mozilla.org/mozilla-central/rev/4d66223323b4 https://hg.mozilla.org/mozilla-central/rev/3beeaf20f46a https://hg.mozilla.org/mozilla-central/rev/34bc72be5d28 https://hg.mozilla.org/mozilla-central/rev/af2f85201ec2

Comment on attachment 8974204 [details] Bug 1457021 - Add l10n migration script for Preferences Subdialog migration to Fluent. https://reviewboard.mozilla.org/r/242496/#review248836 ::: python/l10n/fluent_migrations/bug_1457021_js_subdialogs.py:118 (Diff revision 5) > + ctx.add_transforms( > + 'browser/browser/preferences/siteDataSettings.ftl', > + 'browser/browser/preferences/siteDataSettings.ftl', > + transforms_from( > +""" > +site-usage-persistent = { site-usage-pattern } (Persistent) For posterity: this is broken, and I completely missed it (obviously, it works fine for en-US). Strings in properties: siteUsage=%1$S %2$S siteUsagePersistent=%1$S %2$S (Persistent) String in FTL: site-usage-pattern = { $value } { $unit } site-usage-persistent = { site-usage-pattern } (Persistent) It might not be safe to migrate site-usage-persistent

(In reply to Francesco Lodolo [:flod] from comment #82) > Comment on attachment 8974204 [details] > Bug 1457021 - Add l10n migration script for Preferences Subdialog migration > to Fluent. > > https://reviewboard.mozilla.org/r/242496/#review248836 > > ::: python/l10n/fluent_migrations/bug_1457021_js_subdialogs.py:118 > (Diff revision 5) > > + ctx.add_transforms( > > + 'browser/browser/preferences/siteDataSettings.ftl', > > + 'browser/browser/preferences/siteDataSettings.ftl', > > + transforms_from( > > +""" > > +site-usage-persistent = { site-usage-pattern } (Persistent) > > For posterity: this is broken, and I completely missed it (obviously, it > works fine for en-US). > > Strings in properties: > > siteUsage=%1$S %2$S > siteUsagePersistent=%1$S %2$S (Persistent) > > String in FTL: > > site-usage-pattern = { $value } { $unit } > site-usage-persistent = { site-usage-pattern } (Persistent) > > It might not be safe to migrate site-usage-persistent Looks like only pt-PT changed the translation, removing a space in "%1$S %2$S". It seems safe-ish to migrate replacing "%1$S %2$S" with "{ site-usage-pattern }". https://transvision.mozfr.org/string/?entity=browser/chrome/browser/preferences/preferences.properties:siteUsagePersistent&repo=gecko_strings

(In reply to Francesco Lodolo [:flod] from comment #83) > Looks like only pt-PT changed the translation, removing a space in "%1$S > %2$S". It seems safe-ish to migrate replacing "%1$S %2$S" with "{ > site-usage-pattern }". > > https://transvision.mozfr.org/string/?entity=browser/chrome/browser/ > preferences/preferences.properties:siteUsagePersistent&repo=gecko_strings oc, pl, fr will need a manual fix, since they use a non-breaking space. Definitely need to pay more attention to these cases :-\ https://github.com/flodolo/fluent-migrations/commit/5182773bba416c88562f9474f4e8c8b81a6d03b9

Adding a bit more context, since this might be more confusing that helping. The string is fine site-usage-persistent = { site-usage-pattern } (Persistent) We're replacing "%1$S %2$S" with a message reference to site-usage-pattern, and that improves consistency. What is wrong is the migration recipe that takes the existing strings in .properties, and move them to FTL. The recipe in the bug hardcodes "(Persistent)", and that's my fault. At the same time, replacing "%1$S %2$S" with "{ site-usage-pattern }" is not bullet-proof, so we need to keep it in mind if we decide to do similar change. The easy solution is to simply not migrate such strings, and let them be retranslated.