Make langpacks strip comments and white space from (or minify) `.properties` and `.ftl` files
Categories
(Firefox Build System :: General, enhancement, P3)
Tracking
(firefox99 fixed)
| Tracking | Status | |
|---|---|---|
| firefox99 | --- | fixed |
People
(Reporter: nalexander, Assigned: nalexander)
References
(Blocks 1 open bug)
Details
(Whiteboard: [fidedi-multilocale])
Attachments
(4 files)
I was surprised to discover that the langpacks that we ship to AMO contain comments. For example, the ach langpack from a release build contains
$ cat ./chrome/ach/locale/ach/passwordmgr/passwordmgr.properties
# 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/.
rememberPassword = Tii ki Lalo Mung me donyo pi poo ikom mung me donyo man.
savePasswordTitle = Moki
saveLoginButtonAllow.label = Gwoki
saveLoginButtonAllow.accesskey = G
saveLoginButtonDeny.label = Pe i gwoki
saveLoginButtonDeny.accesskey = e
saveLoginButtonNever.label = Pe i gwok matwal
saveLoginButtonNever.accesskey = e
updateLoginButtonText = Ngec manyen
updateLoginButtonAccessKey = U
updateLoginButtonDeny.label = Pe iket ngec manyen
updateLoginButtonDeny.accesskey = e
# LOCALIZATION NOTE (rememberPasswordMsg):
# 1st string is the username for the login, 2nd is the login's hostname.
# Note that long usernames may be truncated.
rememberPasswordMsg = I mito poo ikom mung me donyo pi “%1$S” i %2$S?
...
and
$ cat browser/localization/ach/devtools/client/aboutdebugging.ftl
# 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/.
### These strings are used inside the about:debugging UI.
# Page Title strings
# Sidebar strings
# Text displayed in the about:debugging sidebar when USB devices discovery is enabled.
about-debugging-sidebar-usb-enabled = Kicako USB
...
Presumably everything compresses well, but it's probably a savings to strip the comments and/or whitespace.
|
Assignee | |
Comment 1•2 years ago
|
||
flod: is there a reason to include comments in langpack files? Is it possible that localizers see the langpack contents as part of their workflow?
|
||
Comment 2•2 years ago
|
||
(In reply to Nick Alexander :nalexander [he/him] from comment #1)
flod: is there a reason to include comments in langpack files? Is it possible that localizers see the langpack contents as part of their workflow?
No, there's no good reason to keep comments in langpacks or in the build.
But it adds complexity and risk, because you need to touch literally all localized files. I don't think that's something that the build system should do, maybe compare-locales. Given the risk is shipping broken builds, this only makes sense if the gain is really significant, and it needs to be bulletproof.
Not sure what you mean with "whitespaces". I wouldn't touch trailing space, and touching empty lines is even more dangerous, given Fluent supports multiline strings (which might include empty lines).
|
||
Comment 3•2 years ago
|
||
Stripping comments from prod files would be a good idea, and should be rather safe if done in/via compare-locales. Even now it's already properly parsing the files while processing them.
To explore the potential benefits, I took the English-language sources of the two examples posted above, and manually removed comments & whitespace. The resulting gzipped file sizes reduced 5023 → 2063 (-59%) for aboutdebugging.ftl and 1342 → 743 (-45%) for passwordmgr.properties.
Conservatively, I would estimate that trimming comments & whitespace would reduce the total payload size of all language data by 30-50% of their current size.
|
|
Assignee | |
Comment 4•2 years ago
|
||
Thanks for the feedback, folks.
For .properties files, this looks like an oversight. When we mach package, the relevant FileFinder has minify=True via --minify and MOZ_PACKAGER_MINIFY. When we mach build installers-..., the relevant FileFinder instances have minify=False. I will try to straighten this out, assuming that it's possible to do so.
For .ftl files, there's no difference between packaging and repackaging. Part of the value proposition of .ftl is that no processing is required, so we might not want to process them to avoid complexity and risk, per #c2.
|
||
Updated•2 years ago
|
|
|
Assignee | |
Comment 5•2 years ago
|
||
There's no current use for setting JS_BINARY in packager.mk, so
remove it while we're here. I elected to make it easy to add new file
types to minify rather than to make it easy to specify JS_BINARY,
since the latter mechanism is strictly more general and could be used
in future for things other than minification.
|
||
Updated•2 years ago
|
|
|
Assignee | |
Comment 6•2 years ago
|
||
Depends on D138364
|
|
Assignee | |
Updated•2 years ago
|
|
||
Updated•2 years ago
|
|
|
Assignee | |
Comment 7•2 years ago
|
||
Depends on D138365
Pushed by nalexander@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/7c29431ee202 Pre: Convert `MOZ_PACKAGER_MINIFY{_JS}` to `moz.configure`. r=firefox-build-system-reviewers,mhentges https://hg.mozilla.org/integration/autoland/rev/a57257eab3fc Make single-locale l10n repacks minify `.properties` files. r=firefox-build-system-reviewers,eemeli,glandium https://hg.mozilla.org/integration/autoland/rev/1ab482292c62 Minify Fluent `.ftl` files in addition to `.properties` files. r=eemeli
|
||
Comment 9•2 years ago
•
|
||
Backed out for GTest crashes on wrappers.cpp
Backout link: https://hg.mozilla.org/integration/autoland/rev/bdaa7a9a4480b22ff8f47d7f4780bcc1fc42977d
Log link: https://treeherder.mozilla.org/logviewer?job_id=369015458&repo=autoland&lineNumber=4176
|
|
Assignee | |
Comment 10•2 years ago
|
||
Well, that's fun: these tests hard-code contents of FTL files: https://searchfox.org/mozilla-central/rev/152a28cc2a64a8efefba2160550e14987b943b74/intl/l10n/rust/gtest/test.rs. We're not supposed to do anything like this but it's probably not worth finding a better test-only solution; I'll just bump the expectations and move on.
|
|
Assignee | |
Comment 11•2 years ago
|
||
|
||
Comment 12•2 years ago
|
||
Pushed by nalexander@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/818337c3ea78 Pre: Make l10n gtests robust to changes to `aboutAbout.ftl`. r=eemeli https://hg.mozilla.org/integration/autoland/rev/a50155ee278b Pre: Convert `MOZ_PACKAGER_MINIFY{_JS}` to `moz.configure`. r=firefox-build-system-reviewers,mhentges https://hg.mozilla.org/integration/autoland/rev/9db4ba547d9b Make single-locale l10n repacks minify `.properties` files. r=firefox-build-system-reviewers,eemeli,glandium https://hg.mozilla.org/integration/autoland/rev/3cfeeea14193 Minify Fluent `.ftl` files in addition to `.properties` files. r=eemeli
|
||
Comment 13•2 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/818337c3ea78
https://hg.mozilla.org/mozilla-central/rev/a50155ee278b
https://hg.mozilla.org/mozilla-central/rev/9db4ba547d9b
https://hg.mozilla.org/mozilla-central/rev/3cfeeea14193
|
|
Assignee | |
Comment 14•2 years ago
|
||
Sadly, whatever benefit this caused in the update MAR files is not apparent -- they all grew 100-200kb in the before and after Nightly builds. But every little bit counts, and this work does make it easier to compare regular packages and single-locale repacks, which was my aim.
|
||
Comment 15•2 years ago
|
||
Given bug 1758568 I think it's pretty risky to enable the JS minifier at all.
Description
•