Make NumberFormat use the ICU currency data, fix bug in NumberFormat

Make NumberFormat use the ICU currency data, fix bug in NumberFormat

NumberFormat previously just used a min of 0 digits after the decimal and a max of 3. This CL changes it so that we use the ICU currency data, and set the min and max to the number of numbers after the decimal point for each currency.

This CL also fixes a small bug where if the minimum fraction digits is above 3 but the maximum fraction digits isn't set, then it returns with only three numbers after the decimal point.

BUG=435465, 473104, 304722
LOG=Y

Committed: https://crrev.com/ddb5c2d999c5ee6e31c4a9599bb3ddb293cc3f49
Cr-Commit-Position: refs/heads/master@{#29734}

[hichris123, 2015-07-16 02:12:14]

hichris123@gmail.com changed reviewers:
+ cira@chromium.org, mnita@google.com

[hichris123, 2015-07-16 02:13:08]

cira@ / mnita@: PTAL at this CL, which makes NumberFormat more accurate for
currencies.

[hichris123, 2015-07-16 19:06:35]

hichris123@gmail.com changed reviewers:
+ jochen@chromium.org

[hichris123, 2015-07-16 19:07:27]

+jochen@ (I can't tell if cira@/mnita@ are still active on chromium. If they are
or you're not a good reviewer for this patch, please let me know)

[jochen (gone - plz use gerrit), 2015-07-17 09:32:48]

looks good, one comment

https://codereview.chromium.org/1231613006/diff/120001/test/intl/number-forma...
File test/intl/number-format/format-currency.js (right):

https://codereview.chromium.org/1231613006/diff/120001/test/intl/number-forma...
test/intl/number-format/format-currency.js:1: // Copyright 2015 the V8 project
authors. All rights reserved.
please use the short form of the license header:

// Copyright 2015 the V8 project authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

[hichris123, 2015-07-17 14:27:06]

https://codereview.chromium.org/1231613006/diff/120001/test/intl/number-forma...
File test/intl/number-format/format-currency.js (right):

https://codereview.chromium.org/1231613006/diff/120001/test/intl/number-forma...
test/intl/number-format/format-currency.js:1: // Copyright 2015 the V8 project
authors. All rights reserved.
On 2015/07/17 09:32:48, jochen wrote:
> please use the short form of the license header:
> 
> // Copyright 2015 the V8 project authors. All rights reserved.
> // Use of this source code is governed by a BSD-style license that can be
> // found in the LICENSE file.

Done.

[jochen (gone - plz use gerrit), 2015-07-17 14:44:30]

The CQ bit was checked by jochen@chromium.org

[jochen (gone - plz use gerrit), 2015-07-17 14:44:30]

lgtm

[commit-bot: I haz the power, 2015-07-17 14:44:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1231613006/140001

[commit-bot: I haz the power, 2015-07-17 14:47:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-17 14:47:22]

Try jobs failed on following builders:
  v8_presubmit on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/4290)

[hichris123, 2015-07-17 14:47:53]

The CQ bit was checked by hichris123@gmail.com

[commit-bot: I haz the power, 2015-07-17 14:48:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1231613006/140001

[commit-bot: I haz the power, 2015-07-17 15:08:00]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-07-17 15:08:16]

Patchset 8 (id:??) landed as
https://crrev.com/ddb5c2d999c5ee6e31c4a9599bb3ddb293cc3f49
Cr-Commit-Position: refs/heads/master@{#29734}

[mnita, 2015-07-17 15:48:54]

I am afraid that this patch breaks functionality in formats other than currency.

https://codereview.chromium.org/1231613006/diff/140001/src/i18n.cc
File src/i18n.cc (right):

https://codereview.chromium.org/1231613006/diff/140001/src/i18n.cc#newcode267
src/i18n.cc:267: 
Two main worries:

1. Looks like the code uses the ICU defaults no matter if the javascript code
specified min/max fraction digits or not.

2. It is not OK to use the currency defaults for all number formatting types.

  2.1 There are currencies that use zero fraction digits by default, because the
currency does not have sub-units.
Example for ja_JP:
   currency: ￥54,306
   number: 54,306.405
   percent: 64%
   scientific: 5.4306404797E4

  2.2 The currency precision is not only locale-dependent, but also currency
dependent. For instance all locales will format the Japanese Yen with not
fractional part. So ucurr_getDefaultFractionDigits, which does not look at the
currency code, will not do the right thing.

Why not just use what ICU returns, "as is", unless caller of the Javascript API
specified the min / max fraction digits?

[hichris123, 2015-07-17 15:54:13]

On 2015/07/17 15:48:54, mnita wrote:
> I am afraid that this patch breaks functionality in formats other than
currency.
> 
> https://codereview.chromium.org/1231613006/diff/140001/src/i18n.cc
> File src/i18n.cc (right):
> 
> https://codereview.chromium.org/1231613006/diff/140001/src/i18n.cc#newcode267
> src/i18n.cc:267: 
> Two main worries:
> 
> 1. Looks like the code uses the ICU defaults no matter if the javascript code
> specified min/max fraction digits or not.
> 
> 2. It is not OK to use the currency defaults for all number formatting types.
> 
>   2.1 There are currencies that use zero fraction digits by default, because
the
> currency does not have sub-units.
> Example for ja_JP:
>    currency: ￥54,306
>    number: 54,306.405
>    percent: 64%
>    scientific: 5.4306404797E4
> 
>   2.2 The currency precision is not only locale-dependent, but also currency
> dependent. For instance all locales will format the Japanese Yen with not
> fractional part. So ucurr_getDefaultFractionDigits, which does not look at the
> currency code, will not do the right thing.
> 
> Why not just use what ICU returns, "as is", unless caller of the Javascript
API
> specified the min / max fraction digits?

Regarding the other-than-currency worries: this *only* sets the fraction digits
if it's a currency. Also, if it is a currency, and someone already specified
min/max fraction digits, it uses those instead of for the currency.

Regarding 2.2: ucurr_getDefaultFractionDigits takes the currency, not the
locale. If the locale does play into how many fraction digits should be shown,
that should probably be changed... but I haven't noticed the locale determining
the fraction digits.

[mnita, 2015-07-17 16:02:39]

Apologies, I have missed the line above [ if (style ==
UNICODE_STRING_SIMPLE("currency")) ]
:-(

Puzzling thing is: why do we even have to do this?
ICU gives the right results, I have tested it outside v8.
I should probably download + build v8 and see what's going on.
Just reading this code does not seem enough for me, right now.
Or it is just Friday morning, and my brain is not fully awake...

[hichris123, 2015-07-17 16:05:34]

On 2015/07/17 16:02:39, mnita wrote:
> Apologies, I have missed the line above [ if (style ==
> UNICODE_STRING_SIMPLE("currency")) ]
> :-(
> 
> Puzzling thing is: why do we even have to do this?
> ICU gives the right results, I have tested it outside v8.
> I should probably download + build v8 and see what's going on.
> Just reading this code does not seem enough for me, right now.
> Or it is just Friday morning, and my brain is not fully awake...

Good morning. :) And sorry for passing you up on the review, I wasn't sure if
you were still contributing to Chromium/V8. I'll make sure to send any further
patches your way.

I think it's because we only pass the locale to ICU, not the currency. I saw on
NumberFormat/DecimalFormat there is a setCurrency() method... but I couldn't
tell if that actually impacted the fractional digits. I figured this would be an
okay way of doing it, but if there's a better way...