1724889 - Minor cleanups in Localization.cpp

Status: RESOLVED FIXED

(Core :: Internationalization: Localization, task)

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1724889 - Add missing includes and clean-up Localization.cpp. r=zbraniecki

Non-unified builds would choke on this file due to the missing includes.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1724889 - Clean up argument conversions to avoid copies and double-work. r=zbraniecki

At least this code seems to be doing twice the work in the common case:

https://searchfox.org/mozilla-central/rev/0fec57c05d3996cc00c55a66f20dd5793a9bfb5d/intl/l10n/Localization.cpp#439,447

Change the code to follow the more common "arrays are out-params" style,
and use in-place construction and early-returning when possible.

Depends on D122203

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2f0e000467f8
Add missing includes and clean-up Localization.cpp. r=zbraniecki
https://hg.mozilla.org/integration/autoland/rev/10d3357fca07
Clean up argument conversions to avoid copies and double-work. r=zbraniecki

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9235544 [details]
Bug 1724889 - Add missing includes and clean-up Localization.cpp. r=zbraniecki

Beta/Release Uplift Approval Request

• User impact if declined: Slower localization
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: none
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Relatively simple cleanup that makes Localization faster.
• String changes made/needed: none

Comment 5

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/2f0e000467f8
https://hg.mozilla.org/mozilla-central/rev/10d3357fca07

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9235544 [details]
Bug 1724889 - Add missing includes and clean-up Localization.cpp. r=zbraniecki

Given that this isn't a new issue in 92, I would be more comfortable with this riding the trains.

Comment 7

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

This is a new issue in 92. It was introduced with the Localization-rs patchset

Comment 8

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

Set release status flags based on info from the regressing bug 1613705

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9235544 [details]
Bug 1724889 - Add missing includes and clean-up Localization.cpp. r=zbraniecki

In the future, it's always a good idea to call out when something's a regression from past behavior as that's definitely a factor we weigh when making approval decisions. Approved for 92.0b5.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f9d5dced5e22
https://hg.mozilla.org/releases/mozilla-beta/rev/ebdc27f69937