Closed Bug 1005863 Opened 5 years ago Closed 5 years ago

Use props2arrays for langGroups.properties

Categories

(Core :: Internationalization, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: hsivonen, Assigned: poiru, Mentored)

Details

(Keywords: memory-footprint, Whiteboard: [lang=c++][MemShrink:P3][diamond])

Attachments

(3 files)

We currently load langGroups.properties as a string bundle with all the string bundle heap overhead (the structures the string bundle needs in addition to the strings themselves and the potential UTF-16 expansion of the strings). We should use props2arrays.py to put the data into the data segment of libxul without the heap overhead of the string bundle.
Any idea how much this will save?
Whiteboard: [mentor=hsivonen][lang=c++][good first bug][MemShrink] → [mentor=hsivonen][lang=c++][good first bug][MemShrink:P3]
Since it's got a priority rating, I'm flagging this as [diamond], but it doesn't look like a good first bug.
Whiteboard: [mentor=hsivonen][lang=c++][good first bug][MemShrink:P3] → [mentor=hsivonen][lang=c++][MemShrink:P3][diamond]
(In reply to Nicholas Nethercote [:njn] from comment #1)
> Any idea how much this will save?

1.5 KB in UTF-16 overhead, unknown amount of malloc slop and 168 times the heap overhead of a hashtable entry.

Let's guess 2 KB.
(In reply to Mike Hoye [:mhoye] from comment #2)
> Since it's got a priority rating, I'm flagging this as [diamond], but it
> doesn't look like a good first bug.

Why not a good first bug?
I've laid out some criteria here: https://wiki.mozilla.org/Contribute/Coding/Mentoring

But in short - good-first bugs should have a well-specified and extremely-narrowly-scoped fix; most of the the challenge is getting your development environment spun up; good next bugs are intended for people who don't need help with that part, and can focus their effort on the bug itself.
Mentor: hsivonen
Whiteboard: [mentor=hsivonen][lang=c++][MemShrink:P3][diamond] → [lang=c++][MemShrink:P3][diamond]
Assignee: nobody → birunthan
Status: NEW → ASSIGNED
Attachment #8473796 - Flags: review?(smontagu)
Comment on attachment 8473796 [details] [diff] [review]
Use props2arrays for langGroups.properties

Review of attachment 8473796 [details] [diff] [review]:
-----------------------------------------------------------------

This looks OK to me, but Henri should probably look at it too.
Attachment #8473796 - Flags: review?(smontagu)
Attachment #8473796 - Flags: review?(hsivonen)
Attachment #8473796 - Flags: review+
Comment on attachment 8473796 [details] [diff] [review]
Use props2arrays for langGroups.properties

The changes here look good, but they are incomplete.

Please also delete the mention of langGroup.properties in
https://mxr.mozilla.org/mozilla-central/source/mobile/android/installer/package-manifest.in#527
https://mxr.mozilla.org/mozilla-central/source/browser/installer/package-manifest.in#714
https://mxr.mozilla.org/mozilla-central/source/b2g/installer/package-manifest.in#705
to save space in what's packaged.

Unfortunately, not packaging langGroups.properties then makes the reference from the Safari Profile Migrator not work:
https://mxr.mozilla.org/mozilla-central/source/browser/components/migration/SafariProfileMigrator.js#466

Since nsILanguageAtomService is not an XPIDL interface, the easiest fix is probably not to migrate the font settings from Safari. We already don't migrate font settings from Chrome and only migrate the x-western font from IE. However, it's probably best to get guidance from mano on how to proceed in the Safari migrator case.

mano, are you OK with not migrating the font from Safari or doing the same "x-western-only" thing that the IE migrator does?

Marking r- due to incompleteness. (The changes in the patch are r+ material, though.)
Attachment #8473796 - Flags: review?(hsivonen) → review-
Flags: needinfo?(mano)
mano, ping for ni? above.
So, the migrator Ben Goodger wrote ages ago did migrate font setting, and that's why I made the new js migrator do that as well. Having said that:

1) Bugs like this should not result in behavior changes. That is, generally speaking. Is this bug important enough to be an exception? Are the no other ways to get the langgroup?
2) I think migrating fonts is actually a nice thing to do, and at the very least something I would expect to be possible to accomplish in JS. I'd not rush to remove the only API that allows that, if that's case.
Flags: needinfo?(mano)
And, of course, "x-western-only" is a nasty bug.
(In reply to Mano (::mano, needinfo? for any questions; not reading general bugmail) from comment #10)
> So, the migrator Ben Goodger wrote ages ago did migrate font setting, and
> that's why I made the new js migrator do that as well. Having said that:
> 
> 1) Bugs like this should not result in behavior changes. That is, generally
> speaking. Is this bug important enough to be an exception?

It's sad to have the current overhead on all platforms, including mobile ones, just to support Safari profile migration, which runs at most once per profile and just on Mac in practice.

> Are the no other ways to get the langgroup?

Since the non-profile migration-relevant API for this stuff isn't scriptable, what we could do is to add a scriptable method that calls the Gecko-internal API.

Is there an existing catch-all scriptable component for random profile migration-exposed utility methods to which the above-described method could be added?

> 2) I think migrating fonts is actually a nice thing to do

It's nice when the user has actually made changes. When not, it amounts to outsourcing the defaults of our product to a competitor.
Flags: needinfo?(mano)
I will file a separate bug to get rid of langGroups.properties entirely (by either removing Safari font migration or by converting langGroups.properties into a JS object in SafariProfileMigrator.js).
Flags: needinfo?(mano)
Attachment #8544406 - Flags: review?(hsivonen)
Attachment #8544406 - Flags: review?(hsivonen) → review+
https://hg.mozilla.org/mozilla-central/rev/f855ecd1b2b5
https://hg.mozilla.org/mozilla-central/rev/48023b20197d
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Thank you for fixing this! Is there a bug number for the SafariProfileMigrator.js follow-up?
(In reply to Henri Sivonen (:hsivonen) from comment #17)
> Is there a bug number for the SafariProfileMigrator.js follow-up?

Yep, see 1129021. Sori viivästyksestä :)
You need to log in before you can comment on or make changes to this bug.