Closed Bug 1733210 Opened 3 years ago Closed 3 years ago

Migrate mail/locales/en-US/chrome/messenger/app-extension-fields.properties to Fluent

Categories

(Thunderbird :: Add-Ons: General, task)

Tracking

(thunderbird_esr91 unaffected)

RESOLVED FIXED
94 Branch
Tracking Status
thunderbird_esr91 --- unaffected

People

(Reporter: robwu, Assigned: Paenglab)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(2 files)

In bug 1729738, the string descriptions for the default themes have been moved tot Fluent.
Thunderbird should also do that to make sure that the built-in themes have the correct name and description (e.g. in the add-ons manager).
See https://phabricator.services.mozilla.com/D126445 for an example of a migration.

Thunderbird previously relied on the default translation in m-c. Note that it was renamed from "Default theme" to "System theme":
https://searchfox.org/mozilla-central/diff/7062a3faa0792761f01cd6447402c56fd27c2aa9/toolkit/locales/en-US/chrome/global/global-extension-fields.properties#5-7

Thunderbird also copied the strings for the light/dark themes from Firefox to Thunderbird,
from https://searchfox.org/mozilla-central/rev/79f93e7a8b9aa1903f1349f2dd46fb71596f2ae9/browser/locales/en-US/chrome/browser/app-extension-fields.properties#5-11
to https://searchfox.org/comm-central/rev/766210e8cd2ad51cc8fce4b429ae091757238d84/mail/locales/en-US/chrome/messenger/app-extension-fields.properties

In toolkit/mozapps/extensions/internal/XPIDatabase.jsm the FTL is added through const l10n = new Localization(["browser/appExtensionFields.ftl"], true);.

Do you know how we can add or override our file?

Flags: needinfo?(geoff)

@Richard: you can register your own FileSource just like langpacks do. We have toolkit -> browser and you could do the same for toolkit -> mail.

See: https://searchfox.org/mozilla-central/search?q=l10n-registry.manifest&path=

Sorry, I don't know not enough about this things. Better someone else takes this bug.

(In reply to Zibi Braniecki [:zbraniecki][:gandalf] from comment #3)

@Richard: you can register your own FileSource just like langpacks do. We have toolkit -> browser and you could do the same for toolkit -> mail.

I'm not sure how it works either... checking the stuff you referred to now.

With https://hg.mozilla.org/mozilla-central/rev/4a72ff9328e5 there is now in toolkit/mozapps/extensions/internal/XPIDatabase.jsm a reference to browser/appExtensionFields.ftl. Toolkit shouldn't reference browser/.

If things work like I think, we use a l10n-registry.manifest to map toolkit to mail (messenger) yes, but if we have to map browser to mail something seems off. Should the reference be to toolkit/appExtensionFields.ftl?

I don't have a great mental model of how it's supposed to work.
The one I operate under is this:

  1. We have a separation between toolkit and %PRODUCT% where product varies.
  2. We have a bunch of addons "bundled" with each %PRODUCT%
  3. Therefore the toolkit component for managing addons should somehow receive from the %PRODUCT% information of what addons it wants to use. The same mechanism should be used to populate the list of l10n resources used to localize those names and descriptions.

So for 3 I imagine some code early in the startup, before toolkit's AddonManager starts reading addons names/descriptions (why is it doing that eagerly at all? No first UI needs them!), should do something like:

AddonsManager.registerL10nResources(["mail/extensionsNames.ftl"]);

and maybe if a given product also uses browser list:

AddonsManager.registerL10nResources(["mail/extensionsNames.ftl", "browser/extensionsNames.ftl"]);
AddonsManager.registerL10nResources(["mail/extensionsNames.ftl", "toolkit/extensionsNames.ftl"]);

and then when AddonsManager starts reading the names/descriptions it lazy creates new Localization(resourceIds) and uses that to localize the names/descriptions, potentially invalidating the cache when registerL10nResources gets called again?

But that's a lot of hand waving and I'd prefer someone who understands how addons are meant to be registered by product to sketch a model that allows the same approach to work for registering l10n resources used to translate them.

The approach from comment 6 doesn't work, because add-ons are loaded early, before browser UI. The description and name are part of the metadata. A non-user UI part where this info is used is the TelemetryEnvironment.

Putting all of this information in XPIDatabase is a hack.

Instead of trying to add an adhoc localization mechanism to the AddonManager, I would prefer the translations to be defined closer to the add-on package. For example by putting the l10n IDs in the manifest file, maybe initially prototyped for built-in extensions only.

In fact... after typing that idea, it appears that this feature already exists; privileged extensions can use l10n_resources: https://searchfox.org/mozilla-central/rev/d10188f9b4f1c4974264f3925505a0498d346c57/toolkit/components/extensions/test/xpcshell/test_ext_l10n.js#119-147

Screenshots migrated from JSON to Fluent in https://hg.mozilla.org/mozilla-central/rev/89dd3abfbb2a7d94f1530b8dac13acc743140333

Let's wait a week before proceeding here.

In bug 1732797, the default theme is being renamed.

I filed bug 1733466, so that the theme manifests can reference the translations from manifest.json, which solves the issue of registering the ftl file.

It solves the issue for the dark and light themes (since these are Thunderbird-specific), but not for the default theme (which is in toolkit).

I think that the translations for the default theme should move back to toolkit, or there should be a way to somehow register a ftl at browser/ in Thunderbird.

Depends on: 1733466, 1732797

Our Add-On Manager is now broken (not completely, but not good either) and some tests are failing.

(In reply to Rob Wu [:robwu] from comment #8)

Let's wait a week before proceeding here.

Also, merge day is next week. Can we please not have this drag on until it's a problem in two versions?

Regardless of the expected patch that I described in comment 8, Thunderbird will need its own version of https://searchfox.org/mozilla-central/rev/eecd2dd4ba630bea4ce103623a4bfb398065b64b/browser/locales/en-US/browser/appExtensionFields.ftl#7-14 that was introduced in bug 1729738. Note that the currently linked code doesn't include the renamed default theme from bug 1732797, you also need to use that ID.

I don't know what you can do to get Fluent to discover the path (and translations) at browser/appExtensionFields.ftl though.
I hope that someone else cc'd on this bug would know, and otherwise you can ask in https://chat.mozilla.org/#/room/#l10n-dev:mozilla.org

I don't know what you can do to get Fluent to discover the path (and translations) at browser/appExtensionFields.ftl though.

I don't know how Thunderbird registers its localization FileSource, I'd expect analogous file to l10n-registry.manifest to exist for mail omni.ja and then within it, you can have browser/appExtensionFields.ftl just like we have one in browser's (the top directory in relative path is just a directory name, it can live in toolkit, browser or mail FileSource).

Great news in comment 12. I added the file in our locales under browser and it works.

Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #9243954 - Flags: review?(mkmelin+mozilla)

@Paenglab: Thanks a lot for figuring this out!

Comment on attachment 9243954 [details] [diff] [review] 1733210-extension-themes-to-Fluent.patch Review of attachment 9243954 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, let's land this. Still, seems odd but I guess bug 1733466 will take care later.
Attachment #9243954 - Flags: review?(mkmelin+mozilla) → review+

(Attaching for posterity the wip patch I had started out, exploring the Thunderbird l10n-registry.manifest way)

Flags: needinfo?(geoff)
Target Milestone: --- → 94 Branch

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/43e935629371
Port bug 1729738: Migrate usages of extension.(.)@mozilla.org. from properties to Fluent. r=mkmelin

Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED

What about tb_fluent_migrations/bug_1733210_migrate_extension_properties.py?

Do we need to ask flod to run it?

Flags: needinfo?(mkmelin+mozilla)

(In reply to Richard Marti (:Paenglab) from comment #18)

What about tb_fluent_migrations/bug_1733210_migrate_extension_properties.py?

Do we need to ask flod to run it?

Yes.

Flags: needinfo?(mkmelin+mozilla) → needinfo?(francesco.lodolo)

Leaving NI open, will clear when I run it (likely on Monday/Tuesday, as I'm waiting for other strings).

Flags: needinfo?(francesco.lodolo)
Flags: needinfo?(francesco.lodolo)

Migrations are running.

For the future, it would be even better to ping before landing the strings in comm-central, as this blocks me from exposing any other string for Firefox :-\

Flags: needinfo?(francesco.lodolo)
Blocks: tb-fluent
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: