Closed
Bug 724531
Opened 13 years ago
Closed 12 years ago
import ICU library into the mozilla tree
Categories
(Core :: Internationalization, defect)
Core
Internationalization
Tracking
()
RESOLVED
FIXED
mozilla22
People
(Reporter: jfkthame, Assigned: mozillabugs)
References
(Blocks 2 open bugs)
Details
Attachments
(3 files, 4 obsolete files)
1.86 KB,
patch
|
mozillabugs
:
review+
|
Details | Diff | Splinter Review |
137.62 KB,
patch
|
dmandelin
:
review+
|
Details | Diff | Splinter Review |
5.21 KB,
patch
|
mozillabugs
:
review+
|
Details | Diff | Splinter Review |
Import the ICU4C source (see http://site.icu-project.org/download).
Current release is 4.8.1.1, though we should be able to drop in new versions as they're released with minimal disruption.
Note that ICU intends to change the version numbering scheme, so that the next release after the 4.8 series will be 49, then 50, etc.
Reporter | ||
Comment 1•13 years ago
|
||
If this is to be used by the JS engine and also by other parts of Gecko, where in the tree do we want the code to live?
Reporter | ||
Updated•13 years ago
|
Are we sure we want to do this? ICU is *huge*. The copy that chrome ships with is 9.5 MB, which is about 62% of the size of libxul.
Reporter | ||
Comment 3•13 years ago
|
||
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #2)
> Are we sure we want to do this? ICU is *huge*. The copy that chrome ships
> with is 9.5 MB, which is about 62% of the size of libxul.
Yes, ICU is huge.... most of that is data, however. That data would either
(a) replace data we're currently shipping (e.g. unicode character properties, charset mappings, locale-specific formatting); or
(b) support new features that we want to add (e.g. JS i18n APIs), and can't unless we ship the data in some form; or
(c) not be required for any feature we offer, in which case we can customize the ICU build to exclude it.
This was initially motivated by the upcoming ECMAscript globalization APIs that we want to be able to implement, but if we go this way we'll want to replace a bunch of existing code and data with ICU-based implementations, which should do a lot to offset the size of ICU.
It is of course possible we'll decide this isn't the best way forward, but it seems worth investigating, at least.
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → mozillabugs
Comment 4•12 years ago
|
||
Regarding the legal bits, Gervase Markham quoth "Yes, the ICU licence is fine."
Comment 5•12 years ago
|
||
I'm really glad to see this development in Mozilla. I fully agree with what Jonathan wrote and I'm more than willing to help with ICU customization (as I and others at Google have done for Chrome and Android).
Assignee | ||
Comment 6•12 years ago
|
||
This script is based on the one to import Breakpad (toolkit/crashreporter/update-breakpad.sh). It omits major chunks of ICU that are not needed to implement the ECMAScript Internationalization API (bug 769872), but may be needed later to implement other features in Mozilla. When run against the ICU 50.1.1 release sources, the script imports ~3250 files out of ~5350.
Two questions:
- Is a top-level directory "icu" the right place to put ICU, or is there another directory where it fits in?
- I put the ICU sources into a subdirectory icu/icu-src to keep ICU-maintained files separate from Mozilla-maintained ones (and to allow for possibly necessary configure.in and Makefile.in), but it makes for an awkward hierarchy icu/icu-src/source. Should icu and icu-src be merged?
Attachment #697616 -
Flags: feedback?(ted)
Comment 7•12 years ago
|
||
Comment on attachment 697616 [details] [diff] [review]
Script to import ICU library into the Mozilla tree (WIP)
Review of attachment 697616 [details] [diff] [review]:
-----------------------------------------------------------------
Looks reasonable.
Attachment #697616 -
Flags: feedback?(ted) → feedback+
Comment 8•12 years ago
|
||
(In reply to Norbert Lindenberg from comment #6)
> - Is a top-level directory "icu" the right place to put ICU, or is there
> another directory where it fits in?
brendan has to sign off on new top-level directories, but that seems fine to me. Alternately you could put it in intl/icu.
> - I put the ICU sources into a subdirectory icu/icu-src to keep
> ICU-maintained files separate from Mozilla-maintained ones (and to allow for
> possibly necessary configure.in and Makefile.in), but it makes for an
> awkward hierarchy icu/icu-src/source. Should icu and icu-src be merged?
That does sound pretty awkward. For Breakpad we just stick our Makefile.ins alongside the Breakpad code and don't worry about it too much. As long as your update script does things correctly it shouldn't matter.
Assignee | ||
Comment 9•12 years ago
|
||
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #8)
> brendan has to sign off on new top-level directories, but that seems fine to
> me. Alternately you could put it in intl/icu.
I discussed this with Brendan; he prefers intl/icu.
Assignee | ||
Comment 10•12 years ago
|
||
Moved ICU sources to intl/icu and update script into intl/.
Attachment #697616 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Attachment #710032 -
Flags: review?(ted)
Comment 11•12 years ago
|
||
Comment on attachment 710032 [details] [diff] [review]
Script to import ICU library into the Mozilla tree
Review of attachment 710032 [details] [diff] [review]:
-----------------------------------------------------------------
::: intl/update-icu.sh
@@ +1,1 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
needs a #!/bin/sh
Attachment #710032 -
Flags: review?(ted) → review+
Assignee | ||
Comment 12•12 years ago
|
||
Updated with #!/bin/bash. Carrying r+ted.
Attachment #710032 -
Attachment is obsolete: true
Attachment #715028 -
Flags: review+
Comment 13•12 years ago
|
||
Comment on attachment 715028 [details] [diff] [review]
Script to import ICU library into the Mozilla tree
Review of attachment 715028 [details] [diff] [review]:
-----------------------------------------------------------------
::: intl/update-icu.sh
@@ +1,1 @@
> +#!/bin/bash
/bin/sh
Assignee | ||
Comment 14•12 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #13)
> > +#!/bin/bash
>
> /bin/sh
I used /bin/bash because I don't have a non-bash sh to test with, and update-breakpad.sh, after which this script is modeled, is using the same. Why should the script reference a tool that we don't actually use?
Comment 15•12 years ago
|
||
AFAICT your script doesn't have any bashism, and it should work in any posix shell, thus /bin/sh.
Assignee | ||
Comment 16•12 years ago
|
||
Updated per comment 13. Carrying r+ted.
Attachment #715028 -
Attachment is obsolete: true
Attachment #715882 -
Flags: review+
Assignee | ||
Comment 17•12 years ago
|
||
Updated toolkit/content/license.html to contain the relevant portions of
http://source.icu-project.org/repos/icu/icu/trunk/license.html
1) Added intl/icu to the existing section with the ICU license and updated the year.
2) Added a section with the Unicode license. Included a reference to js/src/vm, which also uses Unicode data.
3) Did not add a section for cjdict.txt because we're not currently importing that file.
4) Did not add a section for the Time Zone Database because there's no requirement to do so.
Attachment #718147 -
Flags: review?(gerv)
Assignee | ||
Comment 18•12 years ago
|
||
This attachment is an abbreviated form of the full patch importing the ICU source files; it includes the SVN info with the tag used and the names of all 3239 new files. The full patch is available at
http://lindenbergsoftware.com/mozilla/icu-src.patch.gz
Even in gzip form it's over the 4MB limit for Bugzilla attachments.
The patch was generated by running
$ cd intl
$ ./update-icu.sh http://source.icu-project.org/repos/icu/icu/tags/release-50-1-2
r? requests permission to land the imported ICU sources, not actual review of ICU.
Attachment #718157 -
Flags: review?(dmandelin)
Comment 19•12 years ago
|
||
Comment on attachment 718157 [details] [diff] [review]
Imported ICU source files
Review of attachment 718157 [details] [diff] [review]:
-----------------------------------------------------------------
Just to avoid possible confusion:
- Let's not land this until the licensing file changes are reviewed by Gerv and landed, so we know that the licensing issues have been squared away.
- This r+ covers landing the sources to the tree. Including them in any build is a separate step that we're working through with responsible people for each product.
Attachment #718157 -
Flags: review?(dmandelin) → review+
Comment 20•12 years ago
|
||
Comment on attachment 718147 [details] [diff] [review]
Update license.html
Re: The Unicode License, you only need the parts between "Copyright © 1991-2012 Unicode" and "copyright holder", inclusive. The rest is not necessary. r=gerv with that change.
Gerv
Attachment #718147 -
Flags: review?(gerv) → review+
Assignee | ||
Comment 21•12 years ago
|
||
Updated per comment 20. Carrying r+gerv.
Attachment #718147 -
Attachment is obsolete: true
Attachment #718512 -
Flags: review+
Attachment #718512 -
Flags: checkin?(jwalden+bmo)
Assignee | ||
Updated•12 years ago
|
Attachment #715882 -
Flags: checkin?(jwalden+bmo)
Assignee | ||
Comment 22•12 years ago
|
||
Comment on attachment 718157 [details] [diff] [review]
Imported ICU source files
The updated full patch carrying r+dmandelin is at
http://lindenbergsoftware.com/mozilla/icu-src.patch.gz
Attachment #718157 -
Flags: checkin?(jwalden+bmo)
Comment 23•12 years ago
|
||
Comment 24•12 years ago
|
||
Is this off by default? The last time this was proposed in the newsgroups, I (and several others) raised significant objections about what this would cost in terms of footprint: the last action on that thread was "I'm working on a document discussing the issues and possible solutions, including some that haven't come up in the discussion yet. This will hopefully provide a basis for further discussion and decision."
Comment 25•12 years ago
|
||
bsmedberg: comment 19 might shed some light.
Gerv
Comment 26•12 years ago
|
||
Comment on attachment 715882 [details] [diff] [review]
Script to import ICU library into the Mozilla tree
Yes, I'm told it's off for now. The last I was told was that Asa was coming around on the matter wrt Firefox, and a link to a comment to that effect was provided (I don't remember it offhand); I don't know about anything else.
Attachment #715882 -
Flags: checkin?(jwalden+bmo)
Updated•12 years ago
|
Attachment #718157 -
Flags: checkin?(jwalden+bmo)
Updated•12 years ago
|
Attachment #718512 -
Flags: checkin?(jwalden+bmo)
Comment 27•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/1dde77969a45
https://hg.mozilla.org/mozilla-central/rev/7810fb353f1a
https://hg.mozilla.org/mozilla-central/rev/e17bedcbeb7c
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Assignee | ||
Comment 28•12 years ago
|
||
Benjamin: Yes, this is off. Build integration is under construction in bug 724533, and all the other code for the ECMAScript Internationalization API that we're currently landing is still disabled. Dave, Asa, and others are continuing the discussion about where and how to turn things on.
Jonathan and other internationalizers: I should make clear that I've so far only imported the parts of ICU that the ECMAScript Internationalization API needs. Replacing existing code in other parts of Firefox may require importing more code and data. To do this, you'd modify the update-icu.sh script to delete fewer of the files it gets from the ICU repository. If and when the file cjdict.txt is imported, the license file also needs to be updated with the additional bits for cjdict.
Comment 29•12 years ago
|
||
Assuming someone implements --with-system-icu=, as seems likely, they might need to deal with allocator mismatching for NumberingSystem, in the "part 4" patch in bug 837957, if I understand our allocator-hooking correctly:
<Waldo> does our malloc hooking in Gecko also properly hook new/delete? does it hook either in system libraries, should they be used?
<Waldo> in particular, do things work right if there's a |delete foo| in Gecko, but the corresponding |new Foo| was in a system library?
Noting this here on the entirely off chance that whoever does that will see it, or the review will ensure this issue is addressed, for lack of a better place to note this... :-\
Comment 30•11 years ago
|
||
ICU 52.1 is expected to release tomorrow. We'd like it to get the updated UNICODE bi-di algorithm. Are you able to update the tree with that version? http://site.icu-project.org/
Flags: needinfo?(mozillabugs)
Comment 31•11 years ago
|
||
I'm point for ICU right now. And yes, that should be feasible -- with bug 853301 landing, the next necessary task before shipping with Intl on is to update all the imported libraries, data lists, etc. that haven't been updated since April (because with it not being shipped, why bother yet?). It should be a matter of running through the updating information on <https://wiki.mozilla.org/User:Waldo/Internationalization_API> (will be moved to a better location), and if a new ICU's going to be released really soon, of course we should pick up that version. (This assumes things Just Work with the updated version -- quite possible, but not guaranteed.)
Flags: needinfo?(mozillabugs)
Reporter | ||
Comment 32•11 years ago
|
||
(In reply to Jet Villegas (:jet) from comment #30)
> ICU 52.1 is expected to release tomorrow. We'd like it to get the updated
> UNICODE bi-di algorithm. Are you able to update the tree with that version?
> http://site.icu-project.org/
I don't believe updating ICU will in itself give us the updated bidi algorithm, as we don't currently rely purely on ICU to implement that; we have our own code (see layout/base/nsBidi.{h,cpp}) that will need to be updated.
See also bug 922963.
Comment 33•11 years ago
|
||
(In reply to comment #32)
> (In reply to Jet Villegas (:jet) from comment #30)
> > ICU 52.1 is expected to release tomorrow. We'd like it to get the updated
> > UNICODE bi-di algorithm. Are you able to update the tree with that version?
> > http://site.icu-project.org/
>
> I don't believe updating ICU will in itself give us the updated bidi algorithm,
> as we don't currently rely purely on ICU to implement that; we have our own
> code (see layout/base/nsBidi.{h,cpp}) that will need to be updated.
Yeah, we don't use ICU's bidi algorithm at all, so if that is the only reason to update ICU, it can wait I guess.
Comment 34•11 years ago
|
||
No, we do use ICU's implementation of the bidi algorithm: our code in nsBidi.h/cpp is all originally ICU code, but modified at the time that it was imported to use our calling conventions and an XPCOM wrapper, and with the parts we don't use #ifdef'd out.
As I said in bug 922963, there are two possible ways forward: either update nsBidi from the new version of ICU, or start using ICU instead of nsBidi, in which case updating ICU will give us the new version of the algorithm more or less for free. I think I prefer the second option, and I've just opened bug 924851 for the first part of that.
You need to log in
before you can comment on or make changes to this bug.
Description
•