1621674 - Unify Localization.jsm, mozILocalization and Localization IDL

Status: RESOLVED FIXED

(Core :: Internationalization, task, P2)

Description

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

Currently, we have Localization WebIDL backed by C++ which uses Localization.jsm, but in several cases, the JSM is called directly.

The two use different signatures, and in the effort to prepare for migration to Rust (bug 1613705) I'd like to unify that around IDL and stop using JSM anywhere except from within of Localization.cpp.

Comment 1

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

Attachment: Bug 1621674 - Unify Localization.jsm, mozILocalization and Localization WebIDL.

Comment 2

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

Attachment: Bug 1621674 - Update callsites to use Localization WebIDL.

Depends on D66531

Comment 3

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

Attachment: Bug 1621674 - Update tests to use Localization WebIDL.

Depends on D66548

Comment 4

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

The patch is kinda simple - I basically moved all necessary methods to WebIDL, which uses Localization.cpp, and that in turn uses mozILocalization which uses Localization.jsm.

This means that no code and no tests directly use Localization.jsm or mozILocalization, which enables me to later replace it in bug 1613705.

Comment 5

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

I unified the custom generateBundles and generateBundlesSync to a dictionary which allows for the same easiness when mocking, but eventually will allow for easier plugging of L10nRegistry once its behind WebIDL [0].

[0] https://searchfox.org/mozilla-central/source/intl/l10n/L10nRegistry.jsm#136

Comment 6

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

treeherder: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f6000325015c077f56e5d5fa6ae13ff2599fa75

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a657cda5467e
Unify Localization.jsm, mozILocalization and Localization WebIDL. r=smaug
https://hg.mozilla.org/integration/autoland/rev/939beec809c1
Update callsites to use Localization WebIDL. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/f197d83fd8a1
Update tests to use Localization WebIDL. r=jfkthame

Comment 8

[Razvan Maries]

Backed out for perma failures.

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=296643293&resultStatus=testfailed%2Cbusted%2Cexception&revision=f197d83fd8a1c738ba514d1601af0f2e86dc6b35&searchStr=linux%2C18.04%2Cx64%2Copt%2Cnode%2Ctests%2Csource-test-node-newtab-unit-tests%2Cnode%28newtab%29

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=296643293&repo=autoland&lineNumber=377

Backout: https://hg.mozilla.org/integration/autoland/rev/90e193cc5064eae2004bde7367f2ec77b780b79f

Comment 9

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

We need to update RemoteL10n.test.js.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2f7e9c807020
Unify Localization.jsm, mozILocalization and Localization WebIDL. r=smaug
https://hg.mozilla.org/integration/autoland/rev/0b2fdf331bce
Update callsites to use Localization WebIDL. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/f45de944f20a
Update tests to use Localization WebIDL. r=jfkthame,Mardak

Comment 11

[Stefan Hindli [:stefan_hindli]]

Backed out 3 changesets (Bug 1621674) for causing xpcshell permafailures in browser/extensions/formautofill/test/unit/test_createRecords.js CLOSED TREE

https://treeherder.mozilla.org/#/jobs?repo=autoland&fromchange=71de2075e13ea98ed94f80788a6003980b1a1a3d&searchStr=windows%2C10%2Cx64%2Cdebug%2Cxpcshell%2Ctests%2Ctest-windows10-64%2Fdebug-xpcshell-e10s-2%2Cx%28x2%29&selectedJob=296688042

https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=296688042&repo=autoland&lineNumber=4912

Comment 12

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/989838363bca
Backed out 3 changesets for causing xpcshell permafailures in browser/extensions/formautofill/test/unit/test_createRecords.js CLOSED TREE

Comment 13

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

Attachment: Bug 1621674 - Settle on returning null for missing values/messages out of Localization. r?smaug!

Comment 14

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

The reason it got backed out was because in IDL the UTF8String? means a nullabe string, not string || undefined. And we were pretty inconsistent with undefined vs null in returns from Localization.jsm, so I cleaned it up and now the test passes.

I also switched xpcshell tests for Localization to use stringEqual to make sure we catch any regressions in the null returns, and added a couple tests to capture things like formatMessageSync with a missing element. Unfortunately, one test crashes with mismatched compartment, and I can't figure out why (it uses the same code as DOMLocalization.cpp rooters which don't show this bug and I don't understand why Firefox running in debug or opt doesn't reproduce it). I NI'ed :smaug for that.

:stas - for you I'd just like to verify that for now we're ok coalescing all missing returns to null. (I know it may change once we get to specifying higher level APIs).

Comment 15

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

Got help from Olli - the issue was that since we call JSAPI from C++ directly we need an autorealm. Added it and now all tests pass.

Comment 16

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

new try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=700c8a41e1969c8a8a47a40c1200d54556145ed7

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9800998d06b2
Unify Localization.jsm, mozILocalization and Localization WebIDL. r=smaug
https://hg.mozilla.org/integration/autoland/rev/df386249c4f9
Update callsites to use Localization WebIDL. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/c11b34666d70
Update tests to use Localization WebIDL. r=jfkthame,Mardak
https://hg.mozilla.org/integration/autoland/rev/e2f6f75b7d26
Settle on returning null for missing values/messages out of Localization. r=smaug,stas

Comment 18

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/9800998d06b2
https://hg.mozilla.org/mozilla-central/rev/df386249c4f9
https://hg.mozilla.org/mozilla-central/rev/c11b34666d70
https://hg.mozilla.org/mozilla-central/rev/e2f6f75b7d26