[intl] Implement Intl.PluralRules

[intl] Implement Intl.PluralRules

This patch implements the Stage 3 proposal to add a binding to
ICU exposing a mapping by locale from numbers to plural categories.
This proposal is currently implemented behind a flag in SpiderMonkey
and spec text is available at
https://rawgit.com/caridy/intl-plural-rules-spec/master/index.html

A couple issues with this implementation which we may want to resolve
before shipping:
- It's not clear to me what the right interface is to get the supported
  locales for PluralRules. This implementation copies Mozilla's
  approach of pretending all locales in the system are supported, but
  that might not be correct. ICU bug:
  https://ssl.icu-project.org/trac/ticket/12756
- The Intl code is generally a bit roundabout and duplicated. This
  implementation proceeds in the style of the surrounding code, but
  it would be preferable to do more work to clean up the surrounding
  code before adding too many more features in this style.
- Testing is not so complete; test262 tests have some gaps. Firefox
  has some better tests internally (js/src/tests/Intl/PluralRules);
  I think the best thing here would be to upstream those into test262.

BUG=v8:5601

[Dan Ehrenberg, 2017-03-04 22:04:30]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-04 22:04:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-04 22:05:50]

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

[commit-bot: I haz the power, 2017-03-04 22:05:51]

Dry run: Try jobs failed on following builders:
  v8_android_arm_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...)
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/21982)
  v8_linux64_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_verify_csa_rel_n...)
  v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)
  v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/18463)
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/35981)

[Dan Ehrenberg, 2017-03-05 11:39:00]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-05 11:39:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-05 11:40:20]

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

[commit-bot: I haz the power, 2017-03-05 11:40:22]

Dry run: Try jobs failed on following builders:
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/21983)
  v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/18464)

[Dan Ehrenberg, 2017-03-05 11:46:10]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-05 11:46:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-05 11:47:27]

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

[commit-bot: I haz the power, 2017-03-05 11:47:28]

Dry run: Try jobs failed on following builders:
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/21984)
  v8_linux64_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_verify_csa_rel_n...)
  v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/18465)

[Dan Ehrenberg, 2017-03-05 12:05:44]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-05 12:05:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-05 12:33:46]

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

[commit-bot: I haz the power, 2017-03-05 12:33:47]

Dry run: This issue passed the CQ dry run.

[Dan Ehrenberg, 2017-03-05 13:10:53]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-05 13:11:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Dan Ehrenberg, 2017-03-05 13:11:49]

Description was changed from

==========
[intl] Implement PluralRules

BUG=v8:5601
==========

to

==========
[intl] Implement Intl.PluralRules

This patch implements the Stage 3 proposal to add a binding to
ICU exposing a mapping by locale from numbers to plural categories.
This proposal is currently implemented behind a flag in SpiderMonkey
and spec text is available at
https://rawgit.com/caridy/intl-plural-rules-spec/master/index.html

A couple issues with this implementation which we may want to resolve
before shipping:
- It's not clear to me what the right interface is to get the supported
  locales for PluralRules. This implementation copies Mozilla's
  approach of pretending all locales in the system are supported, but
  that might not be correct. ICU bug:
  https://ssl.icu-project.org/trac/ticket/12756
- The Intl code is generally a bit roundabout and duplicated. This
  implementation proceeds in the style of the surrounding code, but
  it would be preferable to do more work to clean up the surrounding
  code before adding too many more features in this style.

BUG=v8:5601
==========

[Dan Ehrenberg, 2017-03-05 13:11:50]

littledan@chromium.org changed reviewers:
+ jshin@chromium.org, yangguo@chromium.org

[Dan Ehrenberg, 2017-03-05 13:34:39]

Description was changed from

==========
[intl] Implement Intl.PluralRules

This patch implements the Stage 3 proposal to add a binding to
ICU exposing a mapping by locale from numbers to plural categories.
This proposal is currently implemented behind a flag in SpiderMonkey
and spec text is available at
https://rawgit.com/caridy/intl-plural-rules-spec/master/index.html

A couple issues with this implementation which we may want to resolve
before shipping:
- It's not clear to me what the right interface is to get the supported
  locales for PluralRules. This implementation copies Mozilla's
  approach of pretending all locales in the system are supported, but
  that might not be correct. ICU bug:
  https://ssl.icu-project.org/trac/ticket/12756
- The Intl code is generally a bit roundabout and duplicated. This
  implementation proceeds in the style of the surrounding code, but
  it would be preferable to do more work to clean up the surrounding
  code before adding too many more features in this style.

BUG=v8:5601
==========

to

==========
[intl] Implement Intl.PluralRules

This patch implements the Stage 3 proposal to add a binding to
ICU exposing a mapping by locale from numbers to plural categories.
This proposal is currently implemented behind a flag in SpiderMonkey
and spec text is available at
https://rawgit.com/caridy/intl-plural-rules-spec/master/index.html

A couple issues with this implementation which we may want to resolve
before shipping:
- It's not clear to me what the right interface is to get the supported
  locales for PluralRules. This implementation copies Mozilla's
  approach of pretending all locales in the system are supported, but
  that might not be correct. ICU bug:
  https://ssl.icu-project.org/trac/ticket/12756
- The Intl code is generally a bit roundabout and duplicated. This
  implementation proceeds in the style of the surrounding code, but
  it would be preferable to do more work to clean up the surrounding
  code before adding too many more features in this style.
- Testing is not so complete; test262 tests have some gaps. Firefox
  has some better tests internally (js/src/tests/Intl/PluralRules);
  I think the best thing here would be to upstream those into test262.

BUG=v8:5601
==========

[Dan Ehrenberg, 2017-03-05 13:34:56]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-05 13:35:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-05 13:55:11]

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

[commit-bot: I haz the power, 2017-03-05 13:55:12]

Dry run: This issue passed the CQ dry run.

[adamk, 2017-03-16 22:50:03]

adamk@chromium.org changed reviewers:
+ adamk@chromium.org
- yangguo@chromium.org

[adamk, 2017-03-16 22:50:04]

From reading the CL description, it sounds like you think there's some cleanup
to be done in this code. Any reason to prioritize the PluralRules implementation
over that?

It seems to me like we have a lot of tech debt in the Intl code, so I'd lean
towards paying that down (especially where it would make implementing a new
feature more pleasant) over continuing in a style that's thought to be not so
good.

[Dan Ehrenberg, 2017-03-17 09:45:57]

On 2017/03/16 22:50:04, adamk wrote:
> From reading the CL description, it sounds like you think there's some cleanup
> to be done in this code. Any reason to prioritize the PluralRules
implementation
> over that?
> 
> It seems to me like we have a lot of tech debt in the Intl code, so I'd lean
> towards paying that down (especially where it would make implementing a new
> feature more pleasant) over continuing in a style that's thought to be not so
> good.

I'm working on a patch for some of this technical debt and with more planned
after that. I'm fine with holding off on landing this patch until those patches
land; it's just slightly more work for me to do the rebase. But one advantage
would be that people can later look back at the commit and see a feature in the
modern style.

For me, the exercise of implementing this feature to get a better feeling for
what refactorings would be most important to prioritize. For example, now I have
a much clearer idea about what to do with resolvedOptions(). It also seemed
reasonable to prioritize the feature as it's at Stage 3 and Firefox has
implemented it already, and it seems to me like a generally useful thing for
sites. And since it's "just one feature", I don't think it makes the state of
the world so much worse.

[adamk, 2017-03-17 20:46:17]

On 2017/03/17 09:45:57, Dan Ehrenberg wrote:
> On 2017/03/16 22:50:04, adamk wrote:
> > From reading the CL description, it sounds like you think there's some
cleanup
> > to be done in this code. Any reason to prioritize the PluralRules
> implementation
> > over that?
> > 
> > It seems to me like we have a lot of tech debt in the Intl code, so I'd lean
> > towards paying that down (especially where it would make implementing a new
> > feature more pleasant) over continuing in a style that's thought to be not
so
> > good.
> 
> I'm working on a patch for some of this technical debt and with more planned
> after that. I'm fine with holding off on landing this patch until those
patches
> land; it's just slightly more work for me to do the rebase. But one advantage
> would be that people can later look back at the commit and see a feature in
the
> modern style.
> 
> For me, the exercise of implementing this feature to get a better feeling for
> what refactorings would be most important to prioritize. For example, now I
have
> a much clearer idea about what to do with resolvedOptions(). It also seemed
> reasonable to prioritize the feature as it's at Stage 3 and Firefox has
> implemented it already, and it seems to me like a generally useful thing for
> sites. And since it's "just one feature", I don't think it makes the state of
> the world so much worse.

This all sounds reasonable to me. For the cleanup/tech debt patches, what's your
timeline look like? If it's a couple days or a week (modulo TC39 probably), then
I'd say let's land those first. If there's more long-term, then landing this
feature first sounds OK to me.

[Dan Ehrenberg, 2017-03-17 22:59:07]

On 2017/03/17 20:46:17, adamk wrote:
> On 2017/03/17 09:45:57, Dan Ehrenberg wrote:
> > On 2017/03/16 22:50:04, adamk wrote:
> > > From reading the CL description, it sounds like you think there's some
> cleanup
> > > to be done in this code. Any reason to prioritize the PluralRules
> > implementation
> > > over that?
> > > 
> > > It seems to me like we have a lot of tech debt in the Intl code, so I'd
lean
> > > towards paying that down (especially where it would make implementing a
new
> > > feature more pleasant) over continuing in a style that's thought to be not
> so
> > > good.
> > 
> > I'm working on a patch for some of this technical debt and with more planned
> > after that. I'm fine with holding off on landing this patch until those
> patches
> > land; it's just slightly more work for me to do the rebase. But one
advantage
> > would be that people can later look back at the commit and see a feature in
> the
> > modern style.
> > 
> > For me, the exercise of implementing this feature to get a better feeling
for
> > what refactorings would be most important to prioritize. For example, now I
> have
> > a much clearer idea about what to do with resolvedOptions(). It also seemed
> > reasonable to prioritize the feature as it's at Stage 3 and Firefox has
> > implemented it already, and it seems to me like a generally useful thing for
> > sites. And since it's "just one feature", I don't think it makes the state
of
> > the world so much worse.
> 
> This all sounds reasonable to me. For the cleanup/tech debt patches, what's
your
> timeline look like? If it's a couple days or a week (modulo TC39 probably),
then
> I'd say let's land those first. If there's more long-term, then landing this
> feature first sounds OK to me.

It's a little longer than that, maybe a month. I want to avoid putting the Intl
code all in objects.h/objects.cc, and there's TC39 and related work right now.

[jungshik at Google, 2017-05-08 19:16:54]

Sorry for terrible delay. I guess a lot of codes have been shuffled around since
this CL was uploaded. So, rebasing is a bit involved (sorry again for the delay
that makes rebasing more work than usual). 

Overall, looks good with some nits below.

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

https://codereview.chromium.org/2736543002/diff/140001/src/i18n.cc#newcode647
src/i18n.cc:647: }
nit:

NumberFormat::createInstance(....) will return right away if U_FAILURE(status)
is true. So, you don't need this check here. Just one at line 652 would suffice.

https://codereview.chromium.org/2736543002/diff/140001/src/i18n.cc#newcode685
src/i18n.cc:685: if (!U_SUCCESS(status)) return false;
nit: U_FAILURE(status) ? 

Again, checking at line 688 (or inside a loop at line 692) would suffice. 
If you want to know exactly where it fails, you may add
DCHECK(U_SUCCESS(status)) here. ditto for my previous comment about |status|
check.

https://codereview.chromium.org/2736543002/diff/140001/src/i18n.cc#newcode688
src/i18n.cc:688: if (!U_SUCCESS(status)) return false;
nit: U_FAILURE(status)

https://codereview.chromium.org/2736543002/diff/140001/src/i18n.cc#newcode937
src/i18n.cc:937: // Convert BCP47 into ICU locale format.
Need to refactor (may be in another CL if you want to). There are a few more
places with this exact (or almost identical) code.

https://codereview.chromium.org/2736543002/diff/140001/src/i18n.h
File src/i18n.h (right):

https://codereview.chromium.org/2736543002/diff/140001/src/i18n.h#newcode124
src/i18n.h:124: // Release memory we allocated for the Collator once the JS
object that holds
Collator?  You mean PluralRules, don't you?

https://codereview.chromium.org/2736543002/diff/140001/src/i18n.h#newcode134
src/i18n.h:134: // http://bugs.icu-project.org/trac/ticket/12763
That bug was fixed and is a part of ICU 59.  I'll update ICU to 59.1 before
long, but you don't have to wait for that. We can land first and fix it later
(one ICU is updated to 59.1 for chromium/v8). 

Or you can wait. It's up to you.

https://codereview.chromium.org/2736543002/diff/140001/src/runtime/runtime-i1...
File src/runtime/runtime-i18n.cc (right):

https://codereview.chromium.org/2736543002/diff/140001/src/runtime/runtime-i1...
src/runtime/runtime-i18n.cc:707: //
http://bugs.icu-project.org/trac/ticket/12763
Fixed in ICU 59.1. Either wait for me to update Chrome/V8's ICU to 59.1 (will
happen before long) or land first and fix it later.

https://codereview.chromium.org/2736543002/diff/140001/src/runtime/runtime-i1...
src/runtime/runtime-i18n.cc:714: if (!U_SUCCESS(status)) return
isolate->ThrowIllegalOperation();
again, check at line 717 would suffice. If you want to know where it fails, you
can add DCHECK here.  And can even use u_errorName() to log a human-readable
error description along with DCHECK. (include 'unicode/utypes.h' for
u_errorName() ).

[jungshik at Google, 2017-05-19 05:28:34]

https://codereview.chromium.org/2736543002/diff/140001/src/i18n.h
File src/i18n.h (right):

https://codereview.chromium.org/2736543002/diff/140001/src/i18n.h#newcode134
src/i18n.h:134: // http://bugs.icu-project.org/trac/ticket/12763
On 2017/05/08 19:16:54, jungshik at Google wrote:
> That bug was fixed and is a part of ICU 59.  I'll update ICU to 59.1 before
> long, but you don't have to wait for that. We can land first and fix it later
> (one ICU is updated to 59.1 for chromium/v8). 
> 
> Or you can wait. It's up to you.

FYI, ICU in Chromium tree was updated to 59.1.

[jungshik at Google, 2017-06-09 22:21:44]

On 2017/05/19 05:28:34, jungshik at Google wrote:
> https://codereview.chromium.org/2736543002/diff/140001/src/i18n.h
> File src/i18n.h (right):
> 
> https://codereview.chromium.org/2736543002/diff/140001/src/i18n.h#newcode134
> src/i18n.h:134: // http://bugs.icu-project.org/trac/ticket/12763
> On 2017/05/08 19:16:54, jungshik at Google wrote:
> > That bug was fixed and is a part of ICU 59.  I'll update ICU to 59.1 before
> > long, but you don't have to wait for that. We can land first and fix it
later
> > (one ICU is updated to 59.1 for chromium/v8). 
> > 
> > Or you can wait. It's up to you.
> 
> FYI, ICU in Chromium tree was updated to 59.1.

Dan, just in case you forget this CL :-)

[Dan Ehrenberg, 2017-07-14 08:31:50]

https://codereview.chromium.org/2736543002/diff/140001/src/runtime/runtime-i1...
File src/runtime/runtime-i18n.cc (right):

https://codereview.chromium.org/2736543002/diff/140001/src/runtime/runtime-i1...
src/runtime/runtime-i18n.cc:707: //
http://bugs.icu-project.org/trac/ticket/12763
On 2017/05/08 19:16:54, jungshik at Google wrote:
> Fixed in ICU 59.1. Either wait for me to update Chrome/V8's ICU to 59.1 (will
> happen before long) or land first and fix it later. 

Looks like that API is C-only. I posted a comment on that bug asking for a C++
API; until then, do you think there are any problems from continuing with the
current approach?