Jonathan Kew [:jfkthame] Reporter
	Description • 13 years ago

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.

	Jonathan Kew [:jfkthame] 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?

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 2 • 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.

	Jonathan Kew [:jfkthame] 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.

	David Mandelin [:dmandelin]
	Comment 4 • 12 years ago

Regarding the legal bits, Gervase Markham quoth "Yes, the ICU licence is fine."

	Jungshik Shin
	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).

	Norbert Lindenberg 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?

	(not currently active) Ted Mielczarek
	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.

	(not currently active) Ted Mielczarek
	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.

	Norbert Lindenberg 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.

	Norbert Lindenberg Assignee
	Comment 10 • 11 years ago

Moved ICU sources to intl/icu and update script into intl/.

	(not currently active) Ted Mielczarek
	Comment 11 • 11 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

	Norbert Lindenberg Assignee
	Comment 12 • 11 years ago

Updated with #!/bin/bash. Carrying r+ted.

	Mike Hommey [:glandium]
	Comment 13 • 11 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

	Norbert Lindenberg Assignee
	Comment 14 • 11 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?

	Mike Hommey [:glandium]
	Comment 15 • 11 years ago

AFAICT your script doesn't have any bashism, and it should work in any posix shell, thus /bin/sh.

	Norbert Lindenberg Assignee
	Comment 16 • 11 years ago

Updated per comment 13. Carrying r+ted.

	Norbert Lindenberg Assignee
	Comment 17 • 11 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.

	Norbert Lindenberg Assignee
	Comment 18 • 11 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.

	David Mandelin [:dmandelin]
	Comment 19 • 11 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.

	Gervase Markham [:gerv]
	Comment 20 • 11 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

	Norbert Lindenberg Assignee
	Comment 21 • 11 years ago

Updated per comment 20. Carrying r+gerv.

	Norbert Lindenberg Assignee
	Comment 22 • 11 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

	Jeff Walden [:Waldo]
	Comment 23 • 11 years ago

Here goes nothin':

https://hg.mozilla.org/integration/mozilla-inbound/rev/1dde77969a45
https://hg.mozilla.org/integration/mozilla-inbound/rev/7810fb353f1a
https://hg.mozilla.org/integration/mozilla-inbound/rev/e17bedcbeb7c

	Benjamin Smedberg
	Comment 24 • 11 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."

	Gervase Markham [:gerv]
	Comment 25 • 11 years ago

bsmedberg: comment 19 might shed some light.

Gerv

	Jeff Walden [:Waldo]
	Comment 26 • 11 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.

	Ryan VanderMeulen [:RyanVM]
	Comment 27 • 11 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

	Norbert Lindenberg Assignee
	Comment 28 • 11 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.

	Jeff Walden [:Waldo]
	Comment 29 • 11 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...  :-\

	Jet Villegas (inactive)
	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/

	Jeff Walden [:Waldo]
	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.)

	Jonathan Kew [:jfkthame] 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.

	(no longer active)
	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.

	Simon Montagu :smontagu
	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.