1733497 - Migrate region.properties protocol handlers from .properties

Status: RESOLVED FIXED

(Firefox :: File Handling, task, P2)

Description

[Zibi Braniecki [:zbraniecki][:gandalf]]

https://searchfox.org/mozilla-central/source/browser/locales/en-US/chrome/browser-region/region.properties

This is an interesting one. The main issue here is that those are not l10n resources. They're just per-locale meta data files.

I'm wondering if we should move this away from l10n and into some JSON files maintained outside of l10n system.

Comment 1

[Francesco Lodolo [:flod]]

These should definitely live in mozilla-central:

• As you mentioned, they're not l10n resources.
• Localizers are now allowed to make changes without approval, and that's also why they're not exposed in Pontoon.
• Having them in l10n repos forces us to have workarounds all over the place. And sometimes they fail (most recently bug 1730999).

Comment 2

[Zibi Braniecki [:zbraniecki][:gandalf]]

Mike - is that something you may want to tackle? It would help us with "unify l10n" project and clean up tech debt.

Comment 3

[Zibi Braniecki [:zbraniecki][:gandalf]]

Or maybe :mconnor?

Comment 4

[Mike Kaply [:mkaply]]

I've been thinking about this and the only thing that comes to mind is some sort of JSON thing that also might be settable via remote settings?

I'm wondering if Mark has any clever ideas.

Comment 5

[Mark Banner (:standard8)]

(In reply to Mike Kaply [:mkaply] from comment #4)

I've been thinking about this and the only thing that comes to mind is some sort of JSON thing that also might be settable via remote settings?

You could use a shipped JSON blob (or just hard-coded JS) without the need for remote settings.

You could use remote settings, but I think it would need consideration of a) if something would cause a change, would it need to be applied to all versions of Firefox, or just the latest? b) how often are we likely to update these?

If we don't need to worry about older versions, and we're not going to be updating frequently, then in-product seems appropriate. Otherwise remote settings might be appropriate.

Comment 6

[Mike Kaply [:mkaply]]

Are these always language specific? Or can they be region specific?

I'm wondering if we need a language/region structure like search.

Comment 7

[Francesco Lodolo [:flod]]

(In reply to Mike Kaply [:mkaply] from comment #6)

Are these always language specific? Or can they be region specific?

So far, they have been locale specific. Having them also region specific would be an improvement, but I'm not sure how high in priority.

These settings change rarely, if ever, and IIRC the whole experience around updates is not great at the moment (e.g. I don't think we remove old handlers).

Comment 8

[Mike Kaply [:mkaply]]

We still have the name translation issue though. I guess we put those in Fluent?

Comment 9

[Francesco Lodolo [:flod]]

(In reply to Mike Kaply [:mkaply] from comment #8)

We still have the name translation issue though. I guess we put those in Fluent?

To be honest, I'm not sure most of them should be translated in the first place. If we need a translation for a specific region, we can still keep it in the JSON.

Comment 10

[Mike Kaply [:mkaply]]

So this code would be added to uriloader/exthandler/HandlerService.js

I think we could do something pretty simple to start since most handlers are the same for different locales.

What's the priority on this?

Comment 11

[Mike Kaply [:mkaply]]

I've got this on my backburner. Need to understand timeline.

Comment 12

[Zibi Braniecki [:zbraniecki][:gandalf]]

What's the priority on this?

I'd estimate it as a high-prio long standing tech debt reduction, so below any business goals, but I'd appreciate if it was put at the top of tech debt reduction cycle as it taps into Platform OKR of I18n cleanup for next year.

Can you move it to the correct component and triage?

Comment 13

[Mike Kaply [:mkaply]]

Done.

Comment 14

[Mike Kaply [:mkaply]]

Does this need to be done before end of year for the i18n work?

Comment 15

[Francesco Lodolo [:flod]]

(In reply to Mike Kaply [:mkaply] from comment #14)

Does this need to be done before end of year for the i18n work?

No, I think the important deadline is doing it before the next ESR, so we don't have to carry over this system for another year.

Comment 16

[Mike Kaply [:mkaply]]

This code is shared between mobile and browser. Any thoughts on how I should handle that?

There aren't a ton of differences, but there are some.

Or should we unify?

Comment 17

[Francesco Lodolo [:flod]]

(In reply to Mike Kaply [:mkaply] from comment #16)

This code is shared between mobile and browser. Any thoughts on how I should handle that?

Is there actually anything using the mobile bits? AFAIK, every mobile browser we ship use some code that lives outside of Gecko.

Comment 18

[Mike Kaply [:mkaply]]

Yeah, I think you're right. None of this is used for mobile. They have lots of old stuff in region.properties.

Comment 19

[Mike Kaply [:mkaply]]

One more interesting thing I ran across.

https://searchfox.org/mozilla-central/source/python/mozbuild/mozbuild/test/test_dotproperties.py

Test for UTF8 in properties files. Originally written to be region.properties specific. Not sure if it needs to be kept around.

Comment 20

[Mike Kaply [:mkaply]]

Attachment: Bug 1733497 - Remove region.properties.

Comment 21

[Mike Kaply [:mkaply]]

Once we get rid of region.properties, are there any localized preferences left?

I'm finding tests in about:config that use localized preferences and they depend on region.properties.

Comment 22

[Francesco Lodolo [:flod]]

(In reply to Mike Kaply [:mkaply] from comment #21)

Once we get rid of region.properties, are there any localized preferences left?

What type of preferences? region.properties was limited to protocol handlers at this point, after we centralized search.

Thunderbird has other things (maps, localized reply header), but Firefox doesn't have anything else left.

Comment 23

[Mike Kaply [:mkaply]]

What type of preferences? region.properties was limited to protocol handlers at this point, after we centralized search.

Any preferences that use nsIPrefLocalizedString. There are a whole set of tests for about:config that use region.properties for testing, but now we don't appear to have any other instances of nsIPrefLocalizedString.

https://searchfox.org/mozilla-central/search?q=PREF_STRING_LOCALIZED_MISSING&path=&case=false&regexp=false

I'm just going to remove those tests because I don't think it's worth creating a fake properties file for something that won't be in about:config anymore anyway.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/71cae431fb0d
Remove region.properties. r=geckoview-reviewers,m_kato,flod

Comment 27

[:Gijs (he/him)]

(In reply to Mike Kaply [:mkaply] from comment #23)

What type of preferences? region.properties was limited to protocol handlers at this point, after we centralized search.

Any preferences that use nsIPrefLocalizedString. There are a whole set of tests for about:config that use region.properties for testing, but now we don't appear to have any other instances of nsIPrefLocalizedString.

https://searchfox.org/mozilla-central/search?q=PREF_STRING_LOCALIZED_MISSING&path=&case=false&regexp=false

I'm just going to remove those tests because I don't think it's worth creating a fake properties file for something that won't be in about:config anymore anyway.

I'm confused, because there are definitely lots of cases left of nsIPrefLocalizedString that your patch doesn't remove (cf. https://searchfox.org/mozilla-central/search?q=nsIPrefLocalizedString ), and indeed https://searchfox.org/mozilla-central/source/browser/locales/en-US/chrome/browser/browser.properties is very much non-empty. Why remove the tests for about:config?

Comment 28

[Mike Kaply [:mkaply]]

I said nsiPrefLocalizedString, but what I specifically meant was complex preferences. We no longer have any complex preferences in about:config which is what was being tested. There are no .properties references in firefox.js or all.js.

Comment 29

[:Gijs (he/him)]

(In reply to Mike Kaply [:mkaply] from comment #28)

I said nsiPrefLocalizedString, but what I specifically meant was complex preferences.

I'm confused - AFAICT the only use for nsIPrefLocalizedString is for complex preferences whose internal values are .properties file URIs, which then get loaded and in which the same pref name is then looked up as a string identifier. Complex prefs, on the other hand, have other "complex" value possibilities, like nsIFile (with a system-specific file path, like here). There's definitely still complex preferences left, even if we did remove all the nsIPrefLocalizedString instances that use .properties files.

We no longer have any complex preferences in about:config which is what was being tested. There are no .properties references in firefox.js or all.js.

I don't think that last bit is true, at least: https://hg.mozilla.org/integration/autoland/file/tip/browser/app/profile/firefox.js#l1394 and https://searchfox.org/mozilla-central/search?q=.properties&path=all.js&case=false&regexp=false are all properties references, AFAICT?

And even if you did remove all of those, for some values we still read the prefs using these constructs, and therefore user profiles may still have such values stored (e.g. for the homepage pref, x-ref https://bugzilla.mozilla.org/show_bug.cgi?id=1490339 - we changed the value in firefox.js/all.js ages ago, but we're still trying to read the value as a .properties file for backwards compat. We should really fix that, but it's not directly related to this bug). As a result, the condition for which those tests appear to be testing (AFAICT, a complex value pointing to a .properties file that doesn't have the relevant identifier) can still occur.

Basically, I think the test removal was too hasty and we should (a) put that back and (b) file a follow-up bug to consider removing nsIPrefLocalizedString as a concept - but that would entail removing all the other consumers, removing the last few instances from all.js and firefox.js, giving Thunderbird a courtesy heads-up, adding a data migration bit for user-branch values, removing remaining .properties files from the locale files we ship, and finally removing the interface.

Am I missing something else?

Comment 30

[Mike Kaply [:mkaply]]

The way those tests worked is by using the presence of an empty nsipreflocalized string which no longer exists because of the removal of the handler code. So I can make them work again, we'll just have to put a dummy preference in browser.properties or intl.properties for the purpose of the test.

and sorry I missed those properties references. I clearly fail at searchfox.

Comment 31

[Mike Kaply [:mkaply]]

Attachment: Bug 1733497 - Use a different preference about:config tests. r?Gijs

Comment 32

[Mike Kaply [:mkaply]]

You're right, I should have searched harder. intl.menuitems.alwaysappendaccesskeys is an empty preference as well, so I could have just changed the reference in head.js to use that preference.

Patch attached that brings back the about:config changes with that preference.

Comment 33

[Francesco Lodolo [:flod]]

(In reply to :Gijs (he/him) from comment #29)

I'm confused - AFAICT the only use for nsIPrefLocalizedString is for complex preferences whose internal values are .properties file URIs, which then get loaded and in which the same pref name is then looked up as a string identifier.

Uhm, if that's the case, then I guess we have at least one case?

pref("browser.startup.homepage", "chrome://browser/locale/browser.properties");

BTW, that's something we should kill with fire (bug 1463544).

Comment 34

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/71cae431fb0d

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cc8323e3b7b6
Use a different preference about:config tests. r=Gijs

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a9d7a52dfa99
Fix lint failure r=fix CLOSED TREE

Comment 37

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/cc8323e3b7b6
https://hg.mozilla.org/mozilla-central/rev/a9d7a52dfa99