Closed Bug 1411012 Opened 7 years ago Closed 6 years ago

Experimentally migrate a small chunk of .properties/.DTD to Fluent

Categories

(Core :: Internationalization, enhancement, P3)

Product:
Core
Component:
Internationalization
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: zbraniecki, Assigned: zbraniecki)

References

Details

Attachments

(1 file, 1 obsolete file)

Bug 1411012 - Migrate a small chunk of Privacy pane in Preferences to Fluent.
59 bytes, text/x-review-board-request
Pike
: review+
mshal
: review+
Details 
As part of our second milestone, we want to perform a migration of a small chunk of code from .properties and .DTD to Fluent.

This will test our systems for migration and ensure that we can handle such a feat.

I'm looking for a good candidate, probably within Preferences codebase.
My criteria:
 - small piece, 5 strings or less
 - all strings very simple, no plural rules or other advanced features required
 - Preferences code (to stick to the component we're likely going to work with soon)
 - (nice to have) mix of .properties and .dtd to showcase migration from x.properties+x.dtd => x.ftl

The challenge so far is that it seems that Preferences code do actually use .properties only when necessary - and by necessary I mean that there's some more sophisticated stuff going on.

In particular:
 - UnitFormat stuff ("5 kb" etc.)
 - PLATFORM() macro (string for Windows vs. for Mac)
 - in-progress vs. done (a single <label> with "Calculating X..." turned into "Disk size usage: 5 kb")
 - DOMOverlays

I'm trying to avoid anything like that. There's one candidate in form of siteDataSettings.js/xul.

If that fails, I may have to give up on .properties in this milestone and just do .dtd -> .ftl migration
Ok, here's an ambitious proposal.

I think it's a very attractive piece because it's small, self contained, mixes .properties and .dtd and is relatively simple.

At the same time it's not trivial because it uses variant selectors and .style on dialog element.

Let's use it as a starting point and see if we want to aim for something that ambitious or look for a simpler one.

Stas - do you think the scope is appropriate for the current stability of Fluent? (I know that the comment syntax may change)
Pike, Flod - do you think it makes sense to aim for this piece?
Matjaz - do you think Pontoon may be ready around Nov 15th for this feature set?
Flags: needinfo?(stas)
Flags: needinfo?(m)
Flags: needinfo?(l10n)
Flags: needinfo?(francesco.lodolo)
It seems self contained enough, but I wonder if it's too much of a step forward, and too soon. 

There is a ton of stuff in there that we haven't used yet, and we learned the hard way to not make assumptions on what works and what doesn't in Pontoon (and it was just attributes at the time).

It also looks like we need the migration part fully working, not sure what's the status of that (completed but never used, I'd guess).

Also: you're removing the associated .dtd and .properties. With cross-channel we're not going to lose the files for another 2 cycles, but I wonder if we should keep them around in case something goes wrong.
Flags: needinfo?(francesco.lodolo)
> It seems self contained enough, but I wonder if it's too much of a step forward, and too soon. 

Yep, I can see this. As I said - it's an ambitious approach. I want to get feedback from you and look for what should limit us here.

> There is a ton of stuff in there that we haven't used yet, and we learned the hard way to not make assumptions on what works and what doesn't in Pontoon (and it was just attributes at the time).

I agree that the biggest question mark about this particular chunk is Pontoon. It seems like that any place where Preferences use .properties is where they need a particular feature like PluralRules, or concatenation etc.

Fluent has a really nice solutions to all of them, but Pontoon may not be ready for those. This means that we can either give up on .properties, and just do the first migration off of .dtd -> .fluent, or we can do some partial migration to a suboptimal .ftl file and then improve that one.

I'm not sure if I like the latter, as it adds friction and revisions to the .ftl file, but I also don't love the former, because I feel like this patch is a nice small example of how Fluent makes things easier.

Anyhoo, I'm going through Preferences code, looking at the features planned for Pontoon for M2, and trying to get a sense what's realistic. :)

> It also looks like we need the migration part fully working, not sure what's the status of that (completed but never used, I'd guess).

Correct, hence the question for Stas. I believe migration is supposed to handle things like this, but of course we first need to settle on the target, and then verify that migration works on it.

> Also: you're removing the associated .dtd and .properties. With cross-channel we're not going to lose the files for another 2 cycles, but I wonder if we should keep them around in case something goes wrong.

That's a good question. Axel, what do you advise?
Comment on attachment 8921259 [details]
Bug 1411012 - Migrate a small chunk of Privacy pane in Preferences to Fluent.

https://reviewboard.mozilla.org/r/192274/#review197524

::: browser/components/preferences/applicationManager.js:103
(Diff revision 3)
>      else if (app instanceof Ci.nsIWebContentHandlerInfo)
>        address = app.uri;
>      document.getElementById("appLocation").value = address;
> -    var bundle = document.getElementById("appManagerBundle");
> -    var appType = app instanceof Ci.nsILocalHandlerApp ? "descriptionLocalApp"
> -                                                       : "descriptionWebApp";
> +    const appTypeElem = document.getElementById("appType");
> +    var type = app instanceof Ci.nsILocalHandlerApp ? "web" : "local";
> +    document.l10n.setAttributes(appTypeElem, "description", { type });

I don't quite know how to define the good practice but somehow I feel like two separate messages were a better choice here. Perhaps it has to do with the fact that the conditional logic is already present in the JS code and it feels like it shouldn't be repeated in the localization.

::: browser/locales/en-US/browser/preferences/applicationManager.ftl:18
(Diff revision 3)
> +
> +description-apps =
> +    { $handle ->
> +        *[protocol] The following applications can be used to handle { $description } links
> +         [webFeed] The following applications can be used to handle Web Feeds
> +         [file] The following applications can be used to handle { $description } content

Perhaps the good practice could be defined as: use a select-expression on $handle if you plan to use $handle in the sentence?

::: browser/locales/en-US/browser/preferences/applicationManager.ftl:19
(Diff revision 3)
> +description-apps =
> +    { $handle ->
> +        *[protocol] The following applications can be used to handle { $description } links
> +         [webFeed] The following applications can be used to handle Web Feeds
> +         [file] The following applications can be used to handle { $description } content
> +    }

There's a period missing from the end of this message.

::: intl/l10n/DOMLocalization.jsm:64
(Diff revision 3)
>        'accesskey', 'aria-label', 'aria-valuetext', 'aria-moz-hint', 'label'
>      ],
>      key: ['key', 'keycode'],
>      textbox: ['placeholder'],
>      toolbarbutton: ['tooltiptext'],
> +    dialog: ['title', 'style'],

Could we use `data-l10n-attrs` introduced in https://github.com/projectfluent/fluent.js/pull/70 rathern than hard-code `style` here?
The syntax scope looks good.

The migration code works but in this particular example it might not handle the old translations well because they use a pattern that is frowned upon in Fluent: the JS code first retrieves one translation and then feeds it to another one as an argument. The migration code can only migrate content that isn't changing. Here, the translation should be modified too.
Flags: needinfo?(stas)
Comment on attachment 8921259 [details]
Bug 1411012 - Migrate a small chunk of Privacy pane in Preferences to Fluent.

https://reviewboard.mozilla.org/r/192274/#review197710

::: browser/locales/en-US/browser/preferences/applicationManager.ftl:8
(Diff revision 3)
> +// file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +
> +app-manager
> +    .title = Application details
> +    .style = width: 30em; min-height: 20em;

We'll need a fluent checker for CSS props in compare-locales before we can use that in fluent translations.

::: browser/locales/en-US/browser/preferences/applicationManager.ftl:18
(Diff revision 3)
> +
> +description-apps =
> +    { $handle ->
> +        *[protocol] The following applications can be used to handle { $description } links
> +         [webFeed] The following applications can be used to handle Web Feeds
> +         [file] The following applications can be used to handle { $description } content

I'd go further. Best practice is to not do that.

Or, to rephrase it, if $handle would show up in the string, it'd needed to be translated, possibly, and that'd be hard.

Also, only use a selector if its use is optional. There's nothing we can do in compare-locales that enforces the use of a selector, or I'd say, we shouldn't do anything that enforces the use of a selector.

::: browser/locales/en-US/browser/preferences/applicationManager.ftl:26
(Diff revision 3)
> +description
> +    .value =
> +        { $type ->
> +           *[web] This web application is hosted at:
> +            [local] This application is located at:
> +        }

Same here, no selector please.
Flags: needinfo?(l10n)
I don't think Pontoon will have good support for strings in the patch by Nov 15th. I suggest we pick something simpler.
Flags: needinfo?(m)
Thanks guys!

I updated the patch to your comments, and I think I'll leave it as a demo of how we would envision a migration to look like once Pontoon supports more of Fluent.

For now, I'll look for a simpler patch for the first migration :)
Comment on attachment 8921259 [details]
Bug 1411012 - Migrate a small chunk of Privacy pane in Preferences to Fluent.

https://reviewboard.mozilla.org/r/192274/#review197838

::: browser/components/preferences/applicationManager.js:30
(Diff revision 4)
> -      var key =
> -        (this.handlerInfo.wrappedHandlerInfo instanceof Ci.nsIMIMEInfo) ? "handleFile"
> -                                                                        : "handleProtocol";
> -        contentText = bundle.getFormattedString(key, [description]);
> -    }
> -    contentText = bundle.getFormattedString("descriptionApplications", [contentText]);
> +      handle =
> +        (this.handlerInfo.wrappedHandlerInfo instanceof Ci.nsIMIMEInfo) ? "file"
> +                                                                        : "protocol";
> +    }
> +    const appDescElement = document.getElementById("appDescription");
> +    document.l10n.setAttributes(appDescElement, "desc-apps-${handle}", {

Typo: this should use backticks if you want to use the `${}` interpolation.

That said, I'd recommend another good practice to add to the list: use full ids as often as possible. Instead of `let handle = …` and `desc-apps-${handle}`, I'd encourage the following:

    if (this.handlerInfo.type == TYPE_MAYBE_FEED) {
        var identifier = "desc-apps-web-feed";
    } else {
        identifier = "desc-apps-file";
    }
    
    document.l10n.setAttributes(appDescElement, identifier, { description };

::: browser/components/preferences/applicationManager.js:31
(Diff revision 4)
> -        (this.handlerInfo.wrappedHandlerInfo instanceof Ci.nsIMIMEInfo) ? "handleFile"
> -                                                                        : "handleProtocol";
> -        contentText = bundle.getFormattedString(key, [description]);
> -    }
> -    contentText = bundle.getFormattedString("descriptionApplications", [contentText]);
> -    document.getElementById("appDescription").textContent = contentText;
> +        (this.handlerInfo.wrappedHandlerInfo instanceof Ci.nsIMIMEInfo) ? "file"
> +                                                                        : "protocol";
> +    }
> +    const appDescElement = document.getElementById("appDescription");
> +    document.l10n.setAttributes(appDescElement, "desc-apps-${handle}", {
> +      description

Perhaps `type` would be a better name for this argument?
Attached patch first-migration.diff (obsolete) — — Splinter Review 
Thoughts on this chunk?
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #15)
> Thoughts on this chunk?

LGTM.
Stas, Pike, Flod - looks good to you?
Flags: needinfo?(stas)
Flags: needinfo?(l10n)
Flags: needinfo?(francesco.lodolo)
Should be OK once we know if/at what point we need bug 1321279.
Flags: needinfo?(l10n)
Comment on attachment 8921712 [details] [diff] [review]
first-migration.diff

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

LGTM, still a general concern about dropping strings from the DTD while moving features to FTL. My gut feeling says that moving them at the bottom of the file would be safer.

::: browser/locales/en-US/browser/preferences/privacy.ftl
@@ +3,5 @@
> +// file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +dnt-description = Send websites a “Do Not Track” signal that you don’t want to be tracked
> +dnt-learn-more = Learn more
> +dnt-default

Can we use a better string ID since we're updating the string? dnt-option-(default|always) would work better for me.
Flags: needinfo?(francesco.lodolo)
Attachment #8921712 - Attachment is obsolete: true
Comment on attachment 8921259 [details]
Bug 1411012 - Migrate a small chunk of Privacy pane in Preferences to Fluent.

Ok, updated the patch to :flod's review and added a migration code.

Not sure where the migration file should land, but I think it's a good moment to get another feedback round :)
Attachment #8921259 - Flags: feedback?(stas)
Attachment #8921259 - Flags: feedback?(l10n)
Attachment #8921259 - Flags: feedback?(francesco.lodolo)
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
Before I forget: noticing an apostrophe in one of the strings, I realized that we need to add this test for Fluent
https://searchfox.org/mozilla-central/source/browser/base/content/test/static/browser_misused_characters_in_strings.js
(In reply to Francesco Lodolo [:flod] from comment #22)
> Before I forget: noticing an apostrophe in one of the strings, I realized
> that we need to add this test for Fluent
> https://searchfox.org/mozilla-central/source/browser/base/content/test/
> static/browser_misused_characters_in_strings.js

To clarify for those unfamiliar: UI strings are supposed to used proper apostrophes and double quotes, instead of straight quotes. 

This test is run on all landings, and a string change using ' or " will be backed out. Exceptions are there for legitimate uses, typically HTML fragments.
Comment on attachment 8921259 [details]
Bug 1411012 - Migrate a small chunk of Privacy pane in Preferences to Fluent.

https://reviewboard.mozilla.org/r/192274/#review203674

I don't have opinions on where the migration spec files should go. I'll leave this up to Axel and Flod to decide. It should go somewhere where mozilla-central's Python can find it.

I suggest to call the file bug_1411012.py.

::: migrate_pref_privacy.py:4
(Diff revision 5)
> +# coding=utf8
> +
> +import fluent.syntax.ast as FTL
> +from fluent.migrate import EXTERNAL_ARGUMENT, COPY, PLURALS, REPLACE_IN_TEXT

This migration only needs the COPY transform.

::: migrate_pref_privacy.py:7
(Diff revision 5)
> +
> +import fluent.syntax.ast as FTL
> +from fluent.migrate import EXTERNAL_ARGUMENT, COPY, PLURALS, REPLACE_IN_TEXT
> +
> +def migrate(ctx):
> +    """Migrate several strings from Preferences:Privacy"""

Please use ", part {index}" in the docstring. The migration might happen in multiple commit in l10n-central depending on the blame data.

::: migrate_pref_privacy.py:12
(Diff revision 5)
> +    """Migrate several strings from Preferences:Privacy"""
> +
> +    ctx.maybe_add_localization(
> +        'browser/chrome/browser/preferences/privacy.dtd')
> +
> +    ctx.add_transforms('browser/browser/preferences/privacy.ftl', 'privacy.ftl', [

The second argument to `add_transforms` is the real path of the en-US file.  Here it should be `"browser/locales/en-US/browser/preferences/privacy.ftl"`.

Cf. https://gist.github.com/stasm/a1eba86c9f2c5726453eb7206647b11a
(In reply to Staś Małolepszy :stas from comment #24)

> The second argument to `add_transforms` is the real path of the en-US file. 
> Here it should be `"browser/locales/en-US/browser/preferences/privacy.ftl"`.

I should add that this assumes the migration is run from the root dir of mozilla-central.
Flags: needinfo?(stas)
Priority: -- → P3
Attachment #8921259 - Flags: feedback?(l10n)
Comment on attachment 8921259 [details]
Bug 1411012 - Migrate a small chunk of Privacy pane in Preferences to Fluent.

It looks good to me.

As for migration files, I think browser/locales/ftlmigration could be a good spot, but I'll leave that to pike.

The alternative would be to follow Devtool's example, but I'm not sure it's needed or clearer
https://hg.mozilla.org/mozilla-central/file/default/python/devtools/migrate-l10n/
Attachment #8921259 - Flags: feedback+
(In reply to Francesco Lodolo [:flod] from comment #27)

> As for migration files, I think browser/locales/ftlmigration could be a good
> spot, but I'll leave that to pike.

Currently, the migrate-l10n.py script takes migration spec files as module names, so I'd suggest we put it somewhere in python/, e.g.

    python/
        fluent_migrations/
            __init__.py
            bug_1411012.py

This will allow passing "fluent_migrations.bug_1411012" as the argument to migrate-l10n.
Quoting a few comments.

> Currently, the migrate-l10n.py script takes migration spec files as module
> names, so I'd suggest we put it somewhere in python/, e.g.

> I should add that this assumes the migration is run from the root dir of mozilla-central.

> The second argument to `add_transforms` is the real path of the en-US file.  
> Here it should be `"browser/locales/en-US/browser/preferences/privacy.ftl"`.

How are we supposed to use these scripts to migrate l10n content in l10n repos? That's their goal in the end, not migrating en-US content in m-c
How we run them depends on where we land the script and the migrations :)  I suggest to put the migrations spec files somewhere in python/ because it's automatically added to $PYTHONPATH by mach python.

I imagine the actual command to look something like this, for Italian:

    $ mach python third_party/python/fluent/tools/migrate/migrate-l10n.py \
        --lang it \
        --reference-dir . \
        --localization-dir /path/to/l10n-central/it \
        fluent_migrations.bug_1411012

mach python automatically adds python/ and third_party/python/ to $PYTHONPATH, making the following required modules available:

    compare_locales
    fluent
    fluent_migrations
Stas, I tried to follow the command, but it seems that the fluent in third_party/python/fluent does not have the ./tools/ directory as per https://searchfox.org/mozilla-central/source/third_party/python/fluent/fluent/migrate

Am I missing something?
Flags: needinfo?(stas)
third_party/python/fluent is currently outdated. We'll need to release a new version and update it in the tree. We'll need to take care to include the tools/ directory this time.

In the meantime, you can simply copy python-fluent's files over for testing purposes.
Flags: needinfo?(stas)
I was able to perform the migration using stas' command with this patch! It seems to work well at runtime as well. Ready for review I believe.
Comment on attachment 8921259 [details]
Bug 1411012 - Migrate a small chunk of Privacy pane in Preferences to Fluent.

https://reviewboard.mozilla.org/r/192274/#review209374

The code looks OK, but the build and python parts should get a review by a build peer.

I think we should add bug components for python/l10n to python/moz.build for sure. Core/Localization sounds like an ok choice to me, though I don't have a really strong opinion.
Attachment #8921259 - Flags: review?(l10n) → review-
:mshal - asking for your r? as per :Pike's comment 36. The build and python changes are that we add `python/l10n/fluent_migrations` directory that we'll be using to run migration scripts.

This patch adds the first migration, which adds a single `bugXXXX.py` file in that directory. Then, the person running a migration does what :stas documented in comment 30 to execute the migration on a given locale.

So, your review is needed as a peer of the build config on the addition of the new directory :)
Comment on attachment 8921259 [details]
Bug 1411012 - Migrate a small chunk of Privacy pane in Preferences to Fluent.

https://reviewboard.mozilla.org/r/192274/#review210262

Adding the new BUG_COMPONENT & python directory is fine. I'm not qualified to review the content of the fluent migration code though. Is that directory just going to be a collection of files named bug_XYZ.py? Would it help if they were named after what they are migrating?
Attachment #8921259 - Flags: review?(mshal) → review+
Pike: with stas we tested the migration using the in-tree fluent and compare-locales 2.5.1 against gecko-strings, it and pl. All works as expected.
Comment on attachment 8921259 [details]
Bug 1411012 - Migrate a small chunk of Privacy pane in Preferences to Fluent.

https://reviewboard.mozilla.org/r/192274/#review211400

Found a couple more issues. I also agree with mshal, we can use better file names for migration.

::: browser/locales/en-US/browser/preferences/privacy.ftl:8
(Diff revision 9)
> +// file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +dnt-description = Send websites a “Do Not Track” signal that you don’t want to be tracked
> +dnt-learn-more = Learn more
> +dnt-option-default
> +    .label = Only when using Tracking Protection

Given that most tools won't show the context here, add a comment that this talks about Do-Not-Track?

Maybe even make this a section? Or give a 0.5 group comment?

::: browser/locales/en-US/browser/preferences/privacy.ftl:9
(Diff revision 9)
> +
> +dnt-description = Send websites a “Do Not Track” signal that you don’t want to be tracked
> +dnt-learn-more = Learn more
> +dnt-option-default
> +    .label = Only when using Tracking Protection
> +dnt-option-always

I think we should use good variable names generally, so no 2-3 letter abbreviations?

donottrack-option-always, etc?

::: python/l10n/fluent_migrations/bug_1411012.py:1
(Diff revision 9)
> +# coding=utf8

I guess these should have a license header? I'd follow our test licensing scheme, I think that's in the spirit between test and code licenses.

::: python/l10n/fluent_migrations/bug_1411012.py:11
(Diff revision 9)
> +
> +def migrate(ctx):
> +    """Bug 1411012 - Migrate several strings from Preferences:Privacy, part {index}"""
> +
> +    ctx.maybe_add_localization(
> +        'browser/chrome/browser/preferences/privacy.dtd')

We don't need this anymore?
Attachment #8921259 - Flags: review?(l10n) → review-
Flagging flod with a needinfo now, though it's probably OK to answer it in a week or so.

I'd love for you guys to sync on the procedures to apply these migrations, including what we do when the migrations are applied. Do we file follow-up bugs to remove them again? If so, when?

Also, as we talked about naming schemes, maybe there's a way to name these in a way that's helpful when applying them, and keeping track of which are applied and which are not?
Flags: needinfo?(francesco.lodolo)
(In reply to Axel Hecht [:Pike] from comment #42)
> I'd love for you guys to sync on the procedures to apply these migrations,
> including what we do when the migrations are applied. Do we file follow-up
> bugs to remove them again? If so, when?

Can we clarify what you mean? Are we talking about removing the old files or something else?

We also need to clarify who's going to run the migration (I have the feeling I know the answer to that…), and who should be around to assist. As stellar as our code is, I don't expect things to run smoothly the first time.
(In reply to Francesco Lodolo [:flod] from comment #43)
> Can we clarify what you mean? Are we talking about removing the old files or
> something else?

Yes, talking about old files.

> We also need to clarify who's going to run the migration (I have the feeling
> I know the answer to that…), and who should be around to assist. As stellar
> as our code is, I don't expect things to run smoothly the first time.

I'm hoping that all'y'all will figure that out without me :-)
(In reply to Axel Hecht [:Pike] from comment #41)
> Given that most tools won't show the context here, add a comment that this
> talks about Do-Not-Track?
> 
> Maybe even make this a section? Or give a 0.5 group comment?

Sounds like a good use case for sections, since we're going to port more strings to this file later.

> I think we should use good variable names generally, so no 2-3 letter
> abbreviations?
> 
> donottrack-option-always, etc?

I agree. Side question: do we want to push dashes vs camelCase as good practice for naming string IDs?
(In reply to Francesco Lodolo [:flod] from comment #45)

> I agree. Side question: do we want to push dashes vs camelCase as good
> practice for naming string IDs?

My preference is to avoid camelCase in Fluent. I'd suggest dashes for ids and perhaps filenames too.
Comment on attachment 8921259 [details]
Bug 1411012 - Migrate a small chunk of Privacy pane in Preferences to Fluent.

https://reviewboard.mozilla.org/r/192274/#review211400

> Given that most tools won't show the context here, add a comment that this talks about Do-Not-Track?
> 
> Maybe even make this a section? Or give a 0.5 group comment?

I updated it to use a section and a comment. We decided with stas not to block this patch on 0.5 and committed to make the 0.5 parser/serializer/migration capable of migrating 0.4 ftl files to 0.5 syntax once its released.

> I think we should use good variable names generally, so no 2-3 letter abbreviations?
> 
> donottrack-option-always, etc?

I have to say that I'm more on the "unique group id" side than "descriptive long id" side because a shorter (while still readable of course) ID has a higher chance of showing up properly in the UI in case of a catastrophic failure.

I don't think it's a big difference here, since Preferences have a good amount of space, but this patch is just setting the tone for the future patches, so it would be good to decide what naming scheme we'll promote.
To give examples of the diff between what Axel is suggesting and what I did:

dnt-foo-bar vs. donottrack-foo-bar
lang-open-file vs. languages-open-file
tp-permissions vs. tracking-protection-permissions
sb-header vs. safe-browsing-header

My take is that if there's a group of messages clustered together in a section related to a particular technology (like "Do Not Track", "Tracking Protection", "Safe Browsing" etc.) then it's ok to give the name as section name and then the name's initials in the message ID.

I understand that Axel would like the full name for each ID.

It's not a major issue for me, so I'm happy to follow if that's the consensus, but wanted to suggest the alternative and check :stas and :flod's opinions before I follow :)
Also, :flod, :stas - suggestion for the migration file name?
Flags: needinfo?(stas)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #47)
> I updated it to use a section and a comment.

Forgot to push or that was on purpose, waiting for feedback on the IDs?

> I have to say that I'm more on the "unique group id" side than "descriptive
> long id" side because a shorter (while still readable of course) ID has a
> higher chance of showing up properly in the UI in case of a catastrophic
> failure.

For this very reason, I believe the chance of space being too small to even display the string ID is minor, compared to the positive impact of having a fully readable string ID. Even in a cluster, dnt and similar abbreviations are hard to understand.

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #48)
> Also, :flod, :stas - suggestion for the migration file name?

I wonder if the folder is going to become even messier with descriptive names (I definitely want the bug in the name).

For example bug_1411012_preferences_privacy_donotrack.py or preferences_privacy_donotrack_bug_1411012.py? It's kind of easy to pick names when you work on preference panes, but what happens when we need to migrate obscure pieces of UI?
Re migration file names, one thing that django does is that it requires the migration module names to start with a number, so it can apply them as sorted list.

That's nice in theory, I was concerned about multiple patches being developed in parallel, and that we wouldn't want to have to re-land patches just because another migration made it to m-c first.

No good ideas here.
For messages, I suggest we use descriptive full-worded dash-separated identifiers:

    do-not-track-option-always
    safe-browsing-header

For migration spec files, I suggest the following scheme:

    bug_1234567_optional_qualifier.py

The optional qualifier can be a one or two word summary of the migration: preferences_containers, about_robots etc.
Flags: needinfo?(stas)
For IDs, I think we should be as specific as required by the MessageContext.

In preferences, that's very specific, because it's a single window covering everything.

In other contexts, I expect IDs to end up being much simpler.
Agree. I hope we'll end up chunking Preferences into multiple documents, but it doesn't hurt to start with specific IDs. I'll update the patch today.
Comment on attachment 8921259 [details]
Bug 1411012 - Migrate a small chunk of Privacy pane in Preferences to Fluent.

:flod, :stas - updated the patch. Can you confirm that it looks good to you?
Flags: needinfo?(francesco.lodolo)
Attachment #8921259 - Flags: feedback?(stas)
Attachment #8921259 - Flags: feedback?(francesco.lodolo)
Comment on attachment 8921259 [details]
Bug 1411012 - Migrate a small chunk of Privacy pane in Preferences to Fluent.

https://reviewboard.mozilla.org/r/192274/#review213070

::: browser/locales/en-US/browser/preferences/privacy.ftl:6
(Diff revision 10)
> +// This Source Code Form is subject to the terms of the Mozilla Public
> +// License, v. 2.0. If a copy of the MPL was not distributed with this
> +// file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +// Do Not Track
> +// 

I'd get rid of this whitespace

::: browser/locales/en-US/browser/preferences/privacy.ftl:8
(Diff revision 10)
> +// file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +// Do Not Track
> +// 
> +// Messages in this section are related to the "Do Not Track" functionality.
> +// Read more at: https://www.mozilla.org/en-US/firefox/dnt/

Drop the en-US from link

https://www.mozilla.org/firefox/dnt/

::: python/l10n/fluent_migrations/bug_1411012_first_migration.py:1
(Diff revision 10)
> +# coding=utf8

Following stas' suggestion, I would rename this file at this point.

e.g. bug_1234567_preferences_donotrack.py
Comment on attachment 8921259 [details]
Bug 1411012 - Migrate a small chunk of Privacy pane in Preferences to Fluent.

A couple of minor nits, but the FTL file looks good to me.

There's one concern regarding migration and 0.5, but that's something I want to iron out in the coming days.
Attachment #8921259 - Flags: feedback?(francesco.lodolo) → feedback+
Comment on attachment 8921259 [details]
Bug 1411012 - Migrate a small chunk of Privacy pane in Preferences to Fluent.

https://reviewboard.mozilla.org/r/192274/#review213280

::: browser/locales/en-US/browser/preferences/privacy.ftl:8
(Diff revision 10)
> +// file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +// Do Not Track
> +// 
> +// Messages in this section are related to the "Do Not Track" functionality.
> +// Read more at: https://www.mozilla.org/en-US/firefox/dnt/

The comment looks a bit forced and redundant. I wonder if we should instead go for something simpler. In 0.5 syntax:

    ## Do Not Track

    do-not-track-description = …

::: python/l10n/fluent_migrations/bug_1411012_first_migration.py:12
(Diff revision 10)
> +import fluent.syntax.ast as FTL
> +from fluent.migrate import COPY
> +
> +
> +def migrate(ctx):
> +    """Bug 1411012 - Migrate several strings from Preferences:Privacy, part {index}"""

How about:

    Bug 1411012 - Migrate Do Not Track preferences to Fluent, part {index}.
Attachment #8921259 - Flags: feedback?(stas) → feedback+
Comment on attachment 8921259 [details]
Bug 1411012 - Migrate a small chunk of Privacy pane in Preferences to Fluent.

https://reviewboard.mozilla.org/r/192274/#review213388

Given the number of open issues by flod, stas, and me, I'll stick with an r- still.

::: browser/locales/en-US/browser/preferences/privacy.ftl:8
(Diff revision 10)
> +// file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +// Do Not Track
> +// 
> +// Messages in this section are related to the "Do Not Track" functionality.
> +// Read more at: https://www.mozilla.org/en-US/firefox/dnt/

Piling on, this comment should be bound to a section in 0.4.

::: python/l10n/fluent_migrations/bug_1411012_first_migration.py:5
(Diff revision 10)
> +# coding=utf8
> +
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.

I've read through https://www.mozilla.org/en-US/MPL/license-policy/#Licensing_of_Mozilla_Code, and I think we should make these PD:

Any copyright is dedicated to the Public Domain.
http://creativecommons.org/publicdomain/zero/1.0/
Attachment #8921259 - Flags: review?(l10n) → review-
(In reply to Francesco Lodolo [:flod] from comment #57)
> There's one concern regarding migration and 0.5, but that's something I want
> to iron out in the coming days.

Sorry, didn't have time yesterday to follow up on this. 

The result of the discussion we had is that upgrading Fluent to 0.5, in particular changing the sigil for comments, is going to be extremely painful, and will break all over the place if not planned carefully. For example, if Pontoon serializes back a file with # in a cross-channel scenario, Beta will choke on it, as will external projects not using an updated version of Fluent.

We should release a version of Fluent that doesn't choke on # when parsing files, and serializes back files with # instead of //. For that, we also need to make sure all users of FTL files are up to date (m-c, m-b, external web projects).

In practice, releasing 0.5 should be a blocker for the preferences migration. It doesn't seem effective to land a 0.4 file with sections, if then we need to migrate it to 0.5 soon after, with a completely different structure.
Comment on attachment 8921259 [details]
Bug 1411012 - Migrate a small chunk of Privacy pane in Preferences to Fluent.

https://reviewboard.mozilla.org/r/192274/#review213554
Comment on attachment 8921259 [details]
Bug 1411012 - Migrate a small chunk of Privacy pane in Preferences to Fluent.

https://reviewboard.mozilla.org/r/192274/#review213280

> The comment looks a bit forced and redundant. I wonder if we should instead go for something simpler. In 0.5 syntax:
> 
>     ## Do Not Track
> 
>     do-not-track-description = …

Do you recommend dropping the part of the comment which references a link to the DNT description? If so, :flod - are you ok with that?
Flags: needinfo?(stas)
Flags: needinfo?(francesco.lodolo)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #62)
> Comment on attachment 8921259 [details]
> Bug 1411012 - Migrate a small chunk of Privacy pane in Preferences to Fluent.
> 
> https://reviewboard.mozilla.org/r/192274/#review213280
> 
> > The comment looks a bit forced and redundant. I wonder if we should instead go for something simpler. In 0.5 syntax:
> > 
> >     ## Do Not Track
> > 
> >     do-not-track-description = …
> 
> Do you recommend dropping the part of the comment which references a link to
> the DNT description? If so, :flod - are you ok with that?

Yes, I'm OK with removing it.
Flags: needinfo?(francesco.lodolo)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #62)
> Do you recommend dropping the part of the comment which references a link to
> the DNT description?

Yes. It doesn't hurt to have it, but I think it's most helpful when it's not one of the top 5 search results on Google (which it is).
Flags: needinfo?(stas)
Depends on: 1426053
Comment on attachment 8921259 [details]
Bug 1411012 - Migrate a small chunk of Privacy pane in Preferences to Fluent.

https://reviewboard.mozilla.org/r/192274/#review214442

Marking my r+, but landing this patch depends on the 0.5 patches landing on the 58-and-newer m-c repos. Just to make that explicit.

Not sure if we should mark this bug somehow, given how often "blocked" isn't really blocking in bugzilla.
Attachment #8921259 - Flags: review?(l10n) → review+
Thanks Axel!

> Not sure if we should mark this bug somehow, given how often "blocked" isn't really blocking in bugzilla

I'm trying to use the "blocked" on bmo according to its original meaning :)
:stas found out that fluent 0.4.1 actually handles the files well! Our robust error recovery makes us handle everything well and at this point we don't believe we need any emergency patch to upstream to beta or 0.5 for this.

So, we believe we're ready as of today!

The current plan is to land this on Jan 3rd when everyone is back and ready to monitor the outcome. I believe it gives us enough time before the merge day to back it out if needed.
No longer depends on: 1426053
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #68)
> :stas found out that fluent 0.4.1 actually handles the files well! Our
> robust error recovery makes us handle everything well and at this point we
> don't believe we need any emergency patch to upstream to beta or 0.5 for
> this.

For reference, this was bug 1426463 which is now not needed.
To summarize the discussion happened over IRC, including also :jaws:
* We want to use the monolithic approach for preferences panes, so this patch should use preferences/preferences.ftl, instead of preferences/privacy.ftl. 
* We'll migrate away from main.ftl when we move the rest of the strings, no need for FTL-to-FTL migration, since we can still migrate the existing string from the DTD file.
* The .ftl file needs to be updated to use // instead of #. This removes 0.5 (or a parser for # comments) as a blocker for first migration.

@zibi
Does it sound right?
Flags: needinfo?(gandalf)
Both flod and I ran the migration locally on all locales. We found two locales which broke because of CR EOLs (bug 1426665); flod already went in and fixed both of them in hg (thanks). It looks like we're ready to carry out the migrations in production when this lands.
Yeah. Updated the patch.
Flags: needinfo?(gandalf)
Comment on attachment 8921259 [details]
Bug 1411012 - Migrate a small chunk of Privacy pane in Preferences to Fluent.

https://reviewboard.mozilla.org/r/192274/#review214878

::: python/l10n/fluent_migrations/bug_1411012_preferences_do_not_track.py:14
(Diff revision 12)
> +
> +def migrate(ctx):
> +    """Bug 1411012 - Migrate Do Not Track preferences to Fluent, part {index}."""
> +
> +    ctx.add_transforms(
> +        'browser/browser/preferences/privacy.ftl',

You need to update also the migration script
Comment on attachment 8921259 [details]
Bug 1411012 - Migrate a small chunk of Privacy pane in Preferences to Fluent.

https://reviewboard.mozilla.org/r/192274/#review215040

::: python/l10n/fluent_migrations/bug_1411012_preferences_do_not_track.py:4
(Diff revision 13)
> +# coding=utf8
> +
> +# Any copyright is dedicated to the Public Domain.
> +# http://creativecommons.org/publicdomain/zero/1.0/

This fails on python linter on try.
Thanks Flod, Axel. Fixed.
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/26e2486488fe
Migrate a small chunk of Privacy pane in Preferences to Fluent. r=mshal,Pike
https://hg.mozilla.org/mozilla-central/rev/26e2486488fe
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: