Closed
Bug 1005863
Opened 10 years ago
Closed 9 years ago
Use props2arrays for langGroups.properties
Categories
(Core :: Internationalization, enhancement)
Core
Internationalization
Tracking
()
RESOLVED
FIXED
mozilla38
People
(Reporter: hsivonen, Assigned: poiru, Mentored)
Details
(Keywords: memory-footprint, Whiteboard: [lang=c++][MemShrink:P3][diamond])
Attachments
(3 files)
4.80 KB,
patch
|
smontagu
:
review+
hsivonen
:
review-
|
Details | Diff | Splinter Review |
5.07 KB,
patch
|
poiru
:
review+
|
Details | Diff | Splinter Review |
2.06 KB,
patch
|
hsivonen
:
review+
|
Details | Diff | Splinter Review |
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.
Comment 1•10 years ago
|
||
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]
Comment 2•10 years ago
|
||
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]
Reporter | ||
Comment 3•10 years ago
|
||
(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.
Reporter | ||
Comment 4•10 years ago
|
||
(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?
Comment 5•10 years ago
|
||
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.
Updated•10 years ago
|
Mentor: hsivonen
Whiteboard: [mentor=hsivonen][lang=c++][MemShrink:P3][diamond] → [lang=c++][MemShrink:P3][diamond]
Assignee | ||
Comment 6•10 years ago
|
||
Comment 7•10 years ago
|
||
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+
Reporter | ||
Comment 8•10 years ago
|
||
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)
Assignee | ||
Comment 9•10 years ago
|
||
mano, ping for ni? above.
Comment 10•10 years ago
|
||
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)
Comment 11•10 years ago
|
||
And, of course, "x-western-only" is a nasty bug.
Reporter | ||
Comment 12•10 years ago
|
||
(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)
Assignee | ||
Comment 14•9 years ago
|
||
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)
Reporter | ||
Updated•9 years ago
|
Attachment #8544406 -
Flags: review?(hsivonen) → review+
Assignee | ||
Comment 15•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f855ecd1b2b5 https://hg.mozilla.org/integration/mozilla-inbound/rev/48023b20197d
Comment 16•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f855ecd1b2b5 https://hg.mozilla.org/mozilla-central/rev/48023b20197d
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Reporter | ||
Comment 17•9 years ago
|
||
Thank you for fixing this! Is there a bug number for the SafariProfileMigrator.js follow-up?
Assignee | ||
Comment 18•9 years ago
|
||
(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.
Description
•