1733466 - Move localizations of built-in themes from XPIDatabase.jsm to the theme manifests

Status: ASSIGNED

(Toolkit :: Add-ons Manager, task, P3)

Description

[Rob Wu [:robwu]]

The name/description strings of built-in themes are currently defined outside of the extension package. There is a hack in XPIDatabase.jsm that overrides the name/description of built-in themes (the mechanism used to be more generic, but in practice it's only used by built-in themes).

In bug 1729738, the strings moved to Fluent, and the general mechanism was replaced with something specific to built-in themes.

In bug 1731652, a bunch of themes were localized with a common template. Unlike the built-in themes, the translations there take parameters. This logic is specific to themes in desktop Firefox, and the logic is unfortunately present in toolkit code (used by Thunderbird and Fenix).

A drawback of this hack is that it is not obvious at all that the add-on's metadata is being overridden elsewhere. In particular, if the string changes, toolkit/ consumers such as Thunderbird may not immediately discover the need to update the strings.

We should use the l10n_resources feature, as I mentioned at https://bugzilla.mozilla.org/show_bug.cgi?id=1733210#c7. To support the translations from bug 1731652, a new feature needs to be introduced (to support parameters in the call to Fluent).

Comment 1

[Rob Wu [:robwu]]

I'd like to attach a patch this week.

Comment 2

[Rob Wu [:robwu]]

This is requiring more efforts than expected:

• The feature ought to only be available to privileged extensions. However, there is a cyclic dependency between loading the manifest and determining the privileged state, which I've described in more detail at bug 1734987.

• Because of the incomplete check for privileged state, localizable fields of the extension (including name and description) cannot be translated with Fluent. That is a hard blocker for this bug, because the goal is to move name/description translations to Fluent.

• The fluentL10n property is currently initialized in ExtensionData's parseManifest. This means that the logic only works at the first run, because at subsequent runs, the Extension subclass will skip parseManifest and read the data from the cache instead. Consequently, calls to the browser.i18n.getMessage will fail to find the translation. "Fortunately" the manifest data is cached too, so this bug is not obvious. While not critical for this bug, I intend to fix it anyway in an effort to avoid surprises in the future.

• As originally mentioned, the current l10n_resources API does not support parameters. The API needs to be expanded.

Comment 3

[Rob Wu [:robwu]]

• The fluentL10n property is currently initialized in ExtensionData's parseManifest. This means that the logic only works at the first run, because at subsequent runs, the Extension subclass will skip parseManifest and read the data from the cache instead. Consequently, calls to the browser.i18n.getMessage will fail to find the translation. "Fortunately" the manifest data is cached too, so this bug is not obvious. While not critical for this bug, I intend to fix it anyway in an effort to avoid surprises in the future.

The i18n API is out of scope for Fluent; when Fluent-based extension localization was introduced in bug 1457865, only some manifest keys were supported. This bug will expand support to any manifest key, but it would be too much (and also unnecessary) work to introduce support for Fluent in the i18n API in this bug. This is especially unnecessary because there is already a "standard" (privileged-only) API to use Fluent in content (bug 1580816). If we ever want a proper extension API, then it would be part of bug 1492593.

Although bug 1457865 was marked as fixed, the requested functionality (localizable name/description with Fluent) was not implemented. Hence I'll mark this bug as a blocker of bug 1457865.

Comment 4

[Rob Wu [:robwu]]

Attachment: Bug 1733466 - Part 1: Add unit tests to verify the name/description of built-in add-ons/themes

To prepare for a refactor that changes the way that names are
determined, this patch updates an existing test
(browser_verify_l10n_strings.js) to verify that the
addon.name/description properties have the expected value.

Since that test only runs on Firefox desktop, I have also added a new
test that confirms that all names of built-in add-ons are non-empty
(test_verify_builtin_addon_metadata.html). This is meant to verify the
correctness of themes on Thunderbird (and add-ons on Firefox for
Android, even though Android does not support extension-based themes).

Comment 5

[Rob Wu [:robwu]]

Attachment: Bug 1733466 - Part 2: Don't throw when extension.localize(null) is used with Fluent + test

When l10n_resources support is enabled for ExtensionData and
extension.localize is called with null (which XPIInstall.jsm does),
then the following error is thrown:
can't access property "replace", str is null

This is undesired, so add a null check before using str, plus tests.

Comment 6

[Rob Wu [:robwu]]

Attachment: Bug 1733466 - Part 3: Let l10n_resources support any manifest key + tests

Thanks to work on previous patches (bug 1734987 in particular),
l10n_resources now supports any manifest key.

This patch updates the comments.

This patch also adds a unit test that verifies that the addon.name
is properly localized with Fluent, since that did not work before.

The unit test also checks the behavior of restarts, since that scenario
is significant, as explained in
https://bugzilla.mozilla.org/show_bug.cgi?id=1733466#c3.

Comment 7

[Rob Wu [:robwu]]

Attachment: Bug 1733466 - Part 4: Support l10n_resources in themes

Comment 8

[Rob Wu [:robwu]]

Attachment: Bug 1733466 - Part 5: Migrate built-in themes (default/light/dark/alpenglow) to l10n_resources

Except colorway themes, because those depend on support for parameters.

Comment 9

[Rob Wu [:robwu]]

I've attached the initial patches (plus four more in bug 1734987).

Support for parameters in l10n_resources, plus migrating colorway themes will be done in follow-up patches (to this bug or in a new one, since it does not need to block this bug).

Comment 10

[Rob Wu [:robwu]]

The progress here is blocked on the need to resolve incompatible logic.

The localization system of extensions allows one to read all available locales at any point of time.
At installation time, all available locales of the extension are enumerated and the result is cached by Extension.jsm (parsing logic in ExtensionData base class and caching logic in Extension subclass).

The Addon Manager enumerates all locales and saves the name/description/etc. in the extension database. This allows the information to be read even without loading/reading the add-on package. This logic was introduced in bug 1192433, the current version of the code is at https://searchfox.org/mozilla-central/rev/eb554e155a28ad36aca62281406757833b9c467a/toolkit/mozapps/extensions/internal/XPIInstall.jsm#524-559.

The Fluent API offers the strings for the current locale. The langpacks from where these translations are sourced may change, e.g. staged before application updates or when the user manually switches to a different locale. It is not meaningful to read and cache the localized strings. At best we could read the L10n ID, store it in the extensions database, and use that to read the actual string from the getters for addon.name/description/etc. That would solve the issue from the Addon Manager's point of view, but not the invalid cached content in Extension.jsm (there are extension APIs that depends on localized manifest data, which may potentially read from the cached (parsed manifest) with incorrect information).

Comment 11

[Rob Wu [:robwu]]

Another (test) issue with the patch is that toolkit code referenced strings from browser/. The following tests fail with the current patch stack:

toolkit/mozapps/extensions/test/xpcshell/test_upgrade.js
toolkit/mozapps/extensions/test/xpcshell/test_sideloads_after_rebuild.js

TEST-UNEXPECTED-FAIL | xpcshell.ini:toolkit/mozapps/extensions/test/xpcshell/test_upgrade.js | test_2 - [test_2 : 271] A promise chain failed to handle a rejection: Localization.formatValueSync: [fluent] Couldn't find a message: extension-default-theme-description - stack: localize/str<@resource://gre/modules/Extension.jsm:1489:43

TEST-UNEXPECTED-FAIL | xpcshell.ini:toolkit/mozapps/extensions/test/xpcshell/test_sideloads_after_rebuild.js | test_sideloads_after_rebuild - [test_sideloads_after_rebuild : 271] A promise chain failed to handle a rejection: Localization.formatValueSync: [fluent] Couldn't find a message: extension-default-theme-description - stack: localize/str<@resource://gre/modules/Extension.jsm:1489:43

Comment 12

[BugBot [:suhaib / :marco/ :calixte]]

The following patches are waiting for review from a reviewer who resigned from the review:

ID	Title	Author	Reviewer Status
D128234	Bug 1733466 - Part 1: Add unit tests to verify the name/description of built-in add-ons/themes	robwu	rpl: Resigned from review
D128236	Bug 1733466 - Part 3: Let l10n_resources support any manifest key + tests	robwu	rpl: Resigned from review
D128237	Bug 1733466 - Part 4: Support l10n_resources in themes	robwu	rpl: Resigned from review

:robwu, could you please find another reviewer or abandon the patch if it is no longer relevant?

For more information, please visit auto_nag documentation.

Comment 13

[Rob Wu [:robwu]]

It's not a priority to land these. The main motivation for making them localizable are colorway themes, but with their removal it's not really that pressing any more (bug 1738331).