Add support for DateTimeFormat.formatToParts

Add support for DateTimeFormat.formatToParts

Spec discussion:  https://github.com/tc39/ecma402/issues/30

It's in stage 4 and Firefox has already implemented it.

For now, it's added to HARMONY_IN_PROGRESS bucket behind
'--datetime-format-to-parts' flag.

BUG=v8:5244
TEST=intl/date-format/date-format-to-parts.js
TEST=test262/intl402/DateTimeFormat/prototype/formatToParts/*

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

[jungshik at Google, 2016-08-24 21:52:05]

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

[jungshik at Google, 2016-08-24 21:52:06]

Dan, can you take a look? 
It works, but may not be idomatic v8. So, it may need a few iterations. 

I also wonder if there's a way to represent 'atomic' strings in v8.

[jungshik at Google, 2016-08-24 23:03:35]

https://codereview.chromium.org/2273953003/diff/20001/src/runtime/runtime-i18...
File src/runtime/runtime-i18n.cc (right):

https://codereview.chromium.org/2273953003/diff/20001/src/runtime/runtime-i18...
src/runtime/runtime-i18n.cc:448: Isolate* isolate) {
I guess it can be converted to lambda function if desired.

https://codereview.chromium.org/2273953003/diff/20001/src/runtime/runtime-i18...
src/runtime/runtime-i18n.cc:495: Handle<Name> value_literal =
factory->NewStringFromStaticChars("value");
Is there a better way to represent the above two constant static strings?

[jungshik at Google, 2016-08-24 23:50:25]

Description was changed from

==========
Add support for DateTimeFormat.formatToParts


BUG=v8:5244
==========

to

==========
Add support for DateTimeFormat.formatToParts

Spec discussion:  https://github.com/tc39/ecma402/issues/30 


BUG=v8:5244
==========

[Dan Ehrenberg, 2016-08-25 16:54:46]

A couple initial comments, but I didn't do a fully detailed review.

https://codereview.chromium.org/2273953003/diff/20001/src/runtime/runtime-i18...
File src/runtime/runtime-i18n.cc (right):

https://codereview.chromium.org/2273953003/diff/20001/src/runtime/runtime-i18...
src/runtime/runtime-i18n.cc:448: Isolate* isolate) {
On 2016/08/24 at 23:03:35, jungshik at google wrote:
> I guess it can be converted to lambda function if desired.

What do you mean?

https://codereview.chromium.org/2273953003/diff/20001/src/runtime/runtime-i18...
src/runtime/runtime-i18n.cc:495: Handle<Name> value_literal =
factory->NewStringFromStaticChars("value");
On 2016/08/24 at 23:03:35, jungshik at google wrote:
> Is there a better way to represent the above two constant static strings?

Yes, you can add them to the INTERNALIZED_STRING_LIST in src/heap-symbols.h

https://codereview.chromium.org/2273953003/diff/40001/src/runtime/runtime-i18...
File src/runtime/runtime-i18n.cc (right):

https://codereview.chromium.org/2273953003/diff/40001/src/runtime/runtime-i18...
src/runtime/runtime-i18n.cc:407: "year",          // UDAT_YEAR_FIELD = 1,
Nit: Maybe use the '[UDAT] = "year",' syntax for clarity and maintainability?

https://codereview.chromium.org/2273953003/diff/40001/src/runtime/runtime-i18...
src/runtime/runtime-i18n.cc:504: if (type == nullptr) {
Omitting these fields seems like strange behavior. I wonder if we should pursue
spec changes to make it so that we don't have these null fields in the above
array, or so we can use an "other" type.

[jungshik at Google, 2016-08-25 17:49:47]

On 2016/08/25 16:54:46, Dan Ehrenberg wrote:
> A couple initial comments, but I didn't do a fully detailed review.
> 
>
https://codereview.chromium.org/2273953003/diff/20001/src/runtime/runtime-i18...
> File src/runtime/runtime-i18n.cc (right):
> 
>
https://codereview.chromium.org/2273953003/diff/20001/src/runtime/runtime-i18...
> src/runtime/runtime-i18n.cc:448: Isolate* isolate) {
> On 2016/08/24 at 23:03:35, jungshik at google wrote:
> > I guess it can be converted to lambda function if desired.
> 
> What do you mean?

Instead of passing some variables, I thought of capturing them in lambda, but
with two arguments gone (thanks to
INTERNALIZED_STRING_LIST), it's not worth tinkering, I think. 
 
>
https://codereview.chromium.org/2273953003/diff/20001/src/runtime/runtime-i18...
> src/runtime/runtime-i18n.cc:495: Handle<Name> value_literal =
> factory->NewStringFromStaticChars("value");
> On 2016/08/24 at 23:03:35, jungshik at google wrote:
> > Is there a better way to represent the above two constant static strings?
> 
> Yes, you can add them to the INTERNALIZED_STRING_LIST in src/heap-symbols.h

Thanks !  'value' is already there. I'll add 'type'. 
> 
>
https://codereview.chromium.org/2273953003/diff/40001/src/runtime/runtime-i18...
> File src/runtime/runtime-i18n.cc (right):
> 
>
https://codereview.chromium.org/2273953003/diff/40001/src/runtime/runtime-i18...
> src/runtime/runtime-i18n.cc:407: "year",          // UDAT_YEAR_FIELD = 1,
> Nit: Maybe use the '[UDAT] = "year",' syntax for clarity and maintainability?
 
Thanks. What do you think of adding 'year', 'weekday', etc to the above
INTERNALIZED_STRING_LIST as well? 

Then, I can have 

const DateTimePartEnum kIcuDateFieldIdToDateTimePartName[] = {

  [UDAT_YEAR_FIELD] = DateTimePartYear,
  [UDAT_HOUR_FIELD] = DateTimePartHour, 
....
}; 

Handle<String> IcuDateFieldIdToDateTimePartName (int32_t field_id, Isolate*
isolate) {
  switch (kICuDateFieldIdToDateTimePartName[field_id]) {
    case: DateTimePartYear: 
      return isolate->factory()->year_string();
    ....
}

Or, we can just skip the indirection via kIcuDateFieldIdToDateTimePartName and
use switch-case directly (it's 37 cases vs 10 cases). 

switch(field_id) {
  case: UDAT_YEAR_FILED:
  case UDAT_EXTENDED_YEAR_FIELD:
  case UDAT_YEAR_NAME_FIELD:
    return isolate->factory()->year_string(); 
  case: UDAT_HOUR1_FIELD:
  case: UDAT_HOUR0_FIELD:
   ...
     return isolate->factory()->hour_string();
.....


>
https://codereview.chromium.org/2273953003/diff/40001/src/runtime/runtime-i18...
> src/runtime/runtime-i18n.cc:504: if (type == nullptr) {
> Omitting these fields seems like strange behavior. I wonder if we should
pursue
> spec changes to make it so that we don't have these null fields in the above
> array, or so we can use an "other" type.

The current DateTimeFormat does not support those fields (e.g. fractional
second, quarter, milliseconds in day, etc). 
In the output of format() and formatToParts(), those fields will not show up
because there's no way to request them in |options| of DateTimeFormat.  If we
want to support them, DateTimeFormat spec has to be revised to add options for
them.

[jungshik at Google, 2016-08-25 22:08:38]

Dan, can you take another look?

[Dan Ehrenberg, 2016-08-25 23:37:29]

On 2016/08/25 at 17:49:47, jshin wrote:
> On 2016/08/25 16:54:46, Dan Ehrenberg wrote:
> > A couple initial comments, but I didn't do a fully detailed review.
> > 
> >
https://codereview.chromium.org/2273953003/diff/20001/src/runtime/runtime-i18...
> > File src/runtime/runtime-i18n.cc (right):
> > 
> >
https://codereview.chromium.org/2273953003/diff/20001/src/runtime/runtime-i18...
> > src/runtime/runtime-i18n.cc:448: Isolate* isolate) {
> > On 2016/08/24 at 23:03:35, jungshik at google wrote:
> > > I guess it can be converted to lambda function if desired.
> > 
> > What do you mean?
> 
> Instead of passing some variables, I thought of capturing them in lambda, but
with two arguments gone (thanks to
> INTERNALIZED_STRING_LIST), it's not worth tinkering, I think. 
>  
> >
https://codereview.chromium.org/2273953003/diff/20001/src/runtime/runtime-i18...
> > src/runtime/runtime-i18n.cc:495: Handle<Name> value_literal =
> > factory->NewStringFromStaticChars("value");
> > On 2016/08/24 at 23:03:35, jungshik at google wrote:
> > > Is there a better way to represent the above two constant static strings?
> > 
> > Yes, you can add them to the INTERNALIZED_STRING_LIST in src/heap-symbols.h
> 
> Thanks !  'value' is already there. I'll add 'type'. 
> > 
> >
https://codereview.chromium.org/2273953003/diff/40001/src/runtime/runtime-i18...
> > File src/runtime/runtime-i18n.cc (right):
> > 
> >
https://codereview.chromium.org/2273953003/diff/40001/src/runtime/runtime-i18...
> > src/runtime/runtime-i18n.cc:407: "year",          // UDAT_YEAR_FIELD = 1,
> > Nit: Maybe use the '[UDAT] = "year",' syntax for clarity and
maintainability?
>  
> Thanks. What do you think of adding 'year', 'weekday', etc to the above
INTERNALIZED_STRING_LIST as well? 
> 
> Then, I can have 
> 
> const DateTimePartEnum kIcuDateFieldIdToDateTimePartName[] = {
> 
>   [UDAT_YEAR_FIELD] = DateTimePartYear,
>   [UDAT_HOUR_FIELD] = DateTimePartHour, 
> ....
> }; 
> 
> Handle<String> IcuDateFieldIdToDateTimePartName (int32_t field_id, Isolate*
isolate) {
>   switch (kICuDateFieldIdToDateTimePartName[field_id]) {
>     case: DateTimePartYear: 
>       return isolate->factory()->year_string();
>     ....
> }
> 
> Or, we can just skip the indirection via kIcuDateFieldIdToDateTimePartName and
use switch-case directly (it's 37 cases vs 10 cases). 
> 
> switch(field_id) {
>   case: UDAT_YEAR_FILED:
>   case UDAT_EXTENDED_YEAR_FIELD:
>   case UDAT_YEAR_NAME_FIELD:
>     return isolate->factory()->year_string(); 
>   case: UDAT_HOUR1_FIELD:
>   case: UDAT_HOUR0_FIELD:
>    ...
>      return isolate->factory()->hour_string();
> .....
> 
> 

I like this last variant!
> >
https://codereview.chromium.org/2273953003/diff/40001/src/runtime/runtime-i18...
> > src/runtime/runtime-i18n.cc:504: if (type == nullptr) {
> > Omitting these fields seems like strange behavior. I wonder if we should
pursue
> > spec changes to make it so that we don't have these null fields in the above
> > array, or so we can use an "other" type.
> 
> The current DateTimeFormat does not support those fields (e.g. fractional
second, quarter, milliseconds in day, etc). 
> In the output of format() and formatToParts(), those fields will not show up
because there's no way to request them in |options| of DateTimeFormat.  If we
want to support them, DateTimeFormat spec has to be revised to add options for
them.

In that case, how about adding UNREACHABLE() then, rather than continue?

[Dan Ehrenberg, 2016-08-25 23:49:30]

littledan@chromium.org changed reviewers:
+ mlippautz@chromium.org

[jungshik at Google, 2016-08-26 18:19:26]

Dan, can you take another look? 
I took care of your review comments.

https://codereview.chromium.org/2273953003/diff/80001/src/heap-symbols.h
File src/heap-symbols.h (left):

https://codereview.chromium.org/2273953003/diff/80001/src/heap-symbols.h#oldc...
src/heap-symbols.h:24: V(string_to_string, "[object String]")                   
       \
Running 'sort -f' over this block shuffled more lines around than I'm adding. 

If that's not desired (sorting by foo_string names), I'll move them back to
where they used to be.

[Dan Ehrenberg, 2016-08-26 19:07:10]

https://codereview.chromium.org/2273953003/diff/80001/src/heap-symbols.h
File src/heap-symbols.h (left):

https://codereview.chromium.org/2273953003/diff/80001/src/heap-symbols.h#oldc...
src/heap-symbols.h:24: V(string_to_string, "[object String]")                   
       \
On 2016/08/26 at 18:19:26, jungshik at google wrote:
> Running 'sort -f' over this block shuffled more lines around than I'm adding. 
> 
> If that's not desired (sorting by foo_string names), I'll move them back to
where they used to be.

The sorting is fine for me.

mlippautz, is it OK to add these 10 strings to the internalized string list, or
would you be concerned about the memory overhead?

https://codereview.chromium.org/2273953003/diff/80001/src/js/i18n.js
File src/js/i18n.js (right):

https://codereview.chromium.org/2273953003/diff/80001/src/js/i18n.js#newcode1831
src/js/i18n.js:1831: AddBoundMethod(Intl.DateTimeFormat, 'formatToParts',
formatDateToParts, 0,
Could you put this feature behind a flag and only add the method when the flag
is turned on?

https://codereview.chromium.org/2273953003/diff/80001/src/runtime/runtime-i18...
File src/runtime/runtime-i18n.cc (right):

https://codereview.chromium.org/2273953003/diff/80001/src/runtime/runtime-i18...
src/runtime/runtime-i18n.cc:441: default:
Could you add a comment explaining why this is UNREACHABLE? It seems like it's
based on a rather subtle property of CLDR/LDML.

[Dan Ehrenberg, 2016-08-29 21:05:07]

lgtm

https://codereview.chromium.org/2273953003/diff/120001/src/js/i18n.js
File src/js/i18n.js (right):

https://codereview.chromium.org/2273953003/diff/120001/src/js/i18n.js#newcode...
src/js/i18n.js:1802: function formatDateToParts(formatter, dateValue) {
Nit: The general V8 convention is to use CamelCase starting with a capital
letter.

[Dan Ehrenberg, 2016-08-29 21:05:30]

Did you run the test262 tests against this patch?

[jungshik at Google, 2016-09-01 00:08:50]

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

[commit-bot: I haz the power, 2016-09-01 00:08:54]

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, 2016-09-01 00:10:23]

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

[commit-bot: I haz the power, 2016-09-01 00:10:24]

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_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  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/11901)
  v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
  v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/11841)
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/22981)

[jungshik at Google, 2016-09-01 05:01:51]

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

[commit-bot: I haz the power, 2016-09-01 05:01:56]

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, 2016-09-01 05:07:03]

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

[commit-bot: I haz the power, 2016-09-01 05:07:04]

Dry run: Try jobs failed on following builders:
  v8_win_compile_dbg on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_compile_dbg/builds/2...)
  v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/13461)

[jungshik at Google, 2016-09-01 17:06:33]

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

[commit-bot: I haz the power, 2016-09-01 17:06: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, 2016-09-01 17:07:50]

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

[commit-bot: I haz the power, 2016-09-01 17:07:51]

Dry run: Try jobs failed on following builders:
  v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  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/11977)
  v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
  v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/8049)

[jungshik at Google, 2016-09-01 17:10:52]

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

[commit-bot: I haz the power, 2016-09-01 17:11:00]

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, 2016-09-01 17:13:10]

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

[commit-bot: I haz the power, 2016-09-01 17:13:11]

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_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)

[jungshik at Google, 2016-09-01 17:13:26]

Description was changed from

==========
Add support for DateTimeFormat.formatToParts

Spec discussion:  https://github.com/tc39/ecma402/issues/30 


BUG=v8:5244
==========

to

==========
Add support for DateTimeFormat.formatToParts

Spec discussion:  https://github.com/tc39/ecma402/issues/30 


BUG=v8:5244
TEST=intl/date-format/date-format-to-parts.js
==========

[jungshik at Google, 2016-09-01 17:14:01]

Description was changed from

==========
Add support for DateTimeFormat.formatToParts

Spec discussion:  https://github.com/tc39/ecma402/issues/30 


BUG=v8:5244
TEST=intl/date-format/date-format-to-parts.js
==========

to

==========
Add support for DateTimeFormat.formatToParts

Spec discussion:  https://github.com/tc39/ecma402/issues/30 

It's in stage 4 and Firefox has already implemented it.


BUG=v8:5244
TEST=intl/date-format/date-format-to-parts.js
==========

[jungshik at Google, 2016-09-01 17:19:53]

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

[commit-bot: I haz the power, 2016-09-01 17:20: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, 2016-09-01 17:24:31]

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

[commit-bot: I haz the power, 2016-09-01 17:24:32]

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_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)

[jungshik at Google, 2016-09-01 17:27:39]

On 2016/08/29 21:05:30, Dan Ehrenberg wrote:
> Did you run the test262 tests against this patch?

I was wondering if there's any test for this in test262, but couldn't find it.
Where is it? I updated to the trunk and run gclient sync to pull test262, but
intl402/DateTimeFormat/* does not have any for formatToParts. 
So, I've just added my own simple test to intl/date-format.

[jungshik at Google, 2016-09-01 17:34:09]

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

[commit-bot: I haz the power, 2016-09-01 17:34: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, 2016-09-01 18:14:21]

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

[commit-bot: I haz the power, 2016-09-01 18:14:22]

Dry run: This issue passed the CQ dry run.

[jungshik at Google, 2016-09-02 09:31:23]

Description was changed from

==========
Add support for DateTimeFormat.formatToParts

Spec discussion:  https://github.com/tc39/ecma402/issues/30 

It's in stage 4 and Firefox has already implemented it.


BUG=v8:5244
TEST=intl/date-format/date-format-to-parts.js
==========

to

==========
Add support for DateTimeFormat.formatToParts

Spec discussion:  https://github.com/tc39/ecma402/issues/30 

It's in stage 4 and Firefox has already implemented it.


BUG=v8:5244
TEST=intl/date-format/date-format-to-parts.js
TEST=test262/intl402/DateTimeFormat/prototype/formatToParts/*
==========

[jungshik at Google, 2016-09-02 09:40:50]

In release build, all the test262 tests pass (except for one minor issue with
two very unusual combination of fields - year and day without month).

In debug build, the following code snippet is executed. Returning false from
here triggers CHECK(). 

#ifdef DEBUG
  if (map() != (is_strict(shared()->language_mode())
                    ? native_context->strict_function_map()
                    : native_context->sloppy_function_map())) {
    return false;
  }
#endif

[jungshik at Google, 2016-09-02 17:00:21]

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

[commit-bot: I haz the power, 2016-09-02 17:00:27]

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, 2016-09-02 17:17:59]

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

[commit-bot: I haz the power, 2016-09-02 17:18:00]

Dry run: Try jobs failed on following builders:
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/8149)
  v8_mac_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng_triggered/bui...)

[jungshik at Google, 2016-09-02 18:01:55]

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

[commit-bot: I haz the power, 2016-09-02 18:02:09]

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, 2016-09-02 18:23:26]

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

[commit-bot: I haz the power, 2016-09-02 18:23:27]

Dry run: Try jobs failed on following builders:
  v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux64_avx2_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng_trig...)

[jungshik at Google, 2016-09-06 17:06:13]

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

[commit-bot: I haz the power, 2016-09-06 17:06:22]

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, 2016-09-06 17:09:11]

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

[commit-bot: I haz the power, 2016-09-06 17:09:12]

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

[jungshik at Google, 2016-09-06 18:00:23]

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

[commit-bot: I haz the power, 2016-09-06 18:00:28]

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

[jungshik at Google, 2016-09-06 18:03:19]

On 2016/09/02 09:40:50, jungshik at google wrote:
> In release build, all the test262 tests pass (except for one minor issue with
> two very unusual combination of fields - year and day without month).
> 
> In debug build, the following code snippet is executed. Returning false from
> here triggers CHECK(). 
> 
> #ifdef DEBUG
>   if (map() != (is_strict(shared()->language_mode())
>                     ? native_context->strict_function_map()
>                     : native_context->sloppy_function_map())) {
>     return false;
>   }
> #endif

I should have called RemovePrototype for formatDateToParts (in i18n.js) that I'm
adding behind flag in datetime-format-to-parts.js. That will also fix half a
dozen test failures in Promise* (PS #17). They all come from
JSFunction::RemovePrototype (#ifdef DEBUG section). 

Dan, can you take another look?

[commit-bot: I haz the power, 2016-09-06 18:28:56]

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

[commit-bot: I haz the power, 2016-09-06 18:28:57]

Dry run: This issue passed the CQ dry run.

[Dan Ehrenberg, 2016-09-06 19:39:29]

Generally looks pretty great, just minor comments.

https://codereview.chromium.org/2273953003/diff/360001/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/2273953003/diff/360001/src/flag-definitions.h...
src/flag-definitions.h:219: V(datetime_format_to_parts,
"Intl.DateTimeFormat.formatToParts") \
Please check this in initially as INPROGRESS; I was just suggesting staging
*locally* to see how the tests do.

https://codereview.chromium.org/2273953003/diff/360001/src/js/datetime-format...
File src/js/datetime-format-to-parts.js (right):

https://codereview.chromium.org/2273953003/diff/360001/src/js/datetime-format...
src/js/datetime-format-to-parts.js:14: 'formatToParts', formatDateToParts]);
Style nit: put the closing ]); on the next line.

https://codereview.chromium.org/2273953003/diff/360001/src/js/i18n.js
File src/js/i18n.js (right):

https://codereview.chromium.org/2273953003/diff/360001/src/js/i18n.js#newcode...
src/js/i18n.js:1800: function formatDateToParts(dateValue) {
Nit: rename to FormatDateToParts

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

https://codereview.chromium.org/2273953003/diff/360001/src/runtime/runtime-i1...
src/runtime/runtime-i18n.cc:327: if (!impl->IsJSObject() ||
impl->IsTheHole(isolate)) {
The hole is not an object, so the second part is subsumed by the first, I
believe.

https://codereview.chromium.org/2273953003/diff/360001/test/intl/assert.js
File test/intl/assert.js (right):

https://codereview.chromium.org/2273953003/diff/360001/test/intl/assert.js#ne...
test/intl/assert.js:31: function classOf(object) {
Where do you use this?

https://codereview.chromium.org/2273953003/diff/360001/test/test262/test262.s...
File test/test262/test262.status (right):

https://codereview.chromium.org/2273953003/diff/360001/test/test262/test262.s...
test/test262/test262.status:462:
'intl402/DateTimeFormat/prototype/formatToParts/main': [SKIP],
Is this the test that failed because LDML didn't have an entry that matched?
Maybe file a bug on test262 if so.

[jungshik at Google, 2016-09-06 20:50:12]

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

[commit-bot: I haz the power, 2016-09-06 20:50:15]

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, 2016-09-06 21:16:52]

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

[commit-bot: I haz the power, 2016-09-06 21:16:53]

Dry run: This issue passed the CQ dry run.

[jungshik at Google, 2016-09-06 21:35:14]

Dan, PTAL. 
Addressed your comments.

https://codereview.chromium.org/2273953003/diff/360001/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/2273953003/diff/360001/src/flag-definitions.h...
src/flag-definitions.h:219: V(datetime_format_to_parts,
"Intl.DateTimeFormat.formatToParts") \
On 2016/09/06 19:39:29, Dan Ehrenberg wrote:
> Please check this in initially as INPROGRESS; I was just suggesting staging
> *locally* to see how the tests do.

Well, for local testing, I don't need it either because I can just pass
--extra-flags=--datetime-format-to-parts to run_test.py script. 

However, it's useful to run tests on trybots. Anyway, I'm moving it back to
progress bucket.

https://codereview.chromium.org/2273953003/diff/360001/src/js/datetime-format...
File src/js/datetime-format-to-parts.js (right):

https://codereview.chromium.org/2273953003/diff/360001/src/js/datetime-format...
src/js/datetime-format-to-parts.js:14: 'formatToParts', formatDateToParts]);
On 2016/09/06 19:39:29, Dan Ehrenberg wrote:
> Style nit: put the closing ]); on the next line.

Done.

https://codereview.chromium.org/2273953003/diff/360001/src/js/i18n.js
File src/js/i18n.js (right):

https://codereview.chromium.org/2273953003/diff/360001/src/js/i18n.js#newcode...
src/js/i18n.js:1800: function formatDateToParts(dateValue) {
On 2016/09/06 19:39:29, Dan Ehrenberg wrote:
> Nit: rename to FormatDateToParts

Done.

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

https://codereview.chromium.org/2273953003/diff/360001/src/runtime/runtime-i1...
src/runtime/runtime-i18n.cc:327: if (!impl->IsJSObject() ||
impl->IsTheHole(isolate)) {
On 2016/09/06 19:39:29, Dan Ehrenberg wrote:
> The hole is not an object, so the second part is subsumed by the first, I
> believe.

Yeah, I guessed so, but was not sure (didn't check). Thank you for the
clarification.

https://codereview.chromium.org/2273953003/diff/360001/test/intl/assert.js
File test/intl/assert.js (right):

https://codereview.chromium.org/2273953003/diff/360001/test/intl/assert.js#ne...
test/intl/assert.js:31: function classOf(object) {
On 2016/09/06 19:39:29, Dan Ehrenberg wrote:
> Where do you use this?

It's used by deepEquals() which is in turn used by assertEquals().

https://codereview.chromium.org/2273953003/diff/360001/test/test262/test262.s...
File test/test262/test262.status (right):

https://codereview.chromium.org/2273953003/diff/360001/test/test262/test262.s...
test/test262/test262.status:462:
'intl402/DateTimeFormat/prototype/formatToParts/main': [SKIP],
On 2016/09/06 19:39:29, Dan Ehrenberg wrote:
> Is this the test that failed because LDML didn't have an entry that matched?
> Maybe file a bug on test262 if so.

Actually, I fixed the test in the latest PS (PS #20). It's kinda ICU bug, but I
worked around it. I'm gonna file a bug against ICU.

[jungshik at Google, 2016-09-06 21:39:48]

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

[commit-bot: I haz the power, 2016-09-06 21:39:58]

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

[Dan Ehrenberg, 2016-09-06 21:55:19]

lgtm

LGTM modulo a one-line fix

https://codereview.chromium.org/2273953003/diff/400001/test/test262/test262.s...
File test/test262/test262.status (left):

https://codereview.chromium.org/2273953003/diff/400001/test/test262/test262.s...
test/test262/test262.status:462:
'intl402/DateTimeFormat/prototype/formatToParts/*': [SKIP],
You probably want to not delete this line.

[jungshik at Google, 2016-09-06 22:05:02]

Description was changed from

==========
Add support for DateTimeFormat.formatToParts

Spec discussion:  https://github.com/tc39/ecma402/issues/30 

It's in stage 4 and Firefox has already implemented it.


BUG=v8:5244
TEST=intl/date-format/date-format-to-parts.js
TEST=test262/intl402/DateTimeFormat/prototype/formatToParts/*
==========

to

==========
Add support for DateTimeFormat.formatToParts

Spec discussion:  https://github.com/tc39/ecma402/issues/30

It's in stage 4 and Firefox has already implemented it.

For now, it's added to HARMONY_IN_PROGRESS bucket behind
'--datetime-format-to-parts' flag.

BUG=v8:5244
TEST=intl/date-format/date-format-to-parts.js
TEST=test262/intl402/DateTimeFormat/prototype/formatToParts/*
==========

[jungshik at Google, 2016-09-06 22:05:39]

On 2016/09/06 21:55:19, Dan Ehrenberg wrote:
> lgtm
> 
> LGTM modulo a one-line fix
> 
>
https://codereview.chromium.org/2273953003/diff/400001/test/test262/test262.s...
> File test/test262/test262.status (left):
> 
>
https://codereview.chromium.org/2273953003/diff/400001/test/test262/test262.s...
> test/test262/test262.status:462:
> 'intl402/DateTimeFormat/prototype/formatToParts/*': [SKIP],
> You probably want to not delete this line.

Oops. forgot to add it back. Thanks.

[jungshik at Google, 2016-09-06 22:05:53]

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

[commit-bot: I haz the power, 2016-09-06 22:06:04]

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

[Dan Ehrenberg, 2016-09-06 22:07:59]

lgtm

[commit-bot: I haz the power, 2016-09-06 22:30:54]

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

[commit-bot: I haz the power, 2016-09-06 22:30:55]

Dry run: This issue passed the CQ dry run.

[jungshik at Google, 2016-09-06 22:54:59]

The CQ bit was checked by jshin@chromium.org

[commit-bot: I haz the power, 2016-09-06 22:55:12]

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, 2016-09-06 22:56:57]

Committed patchset #22 (id:420001)

[commit-bot: I haz the power, 2016-09-06 22:57:18]

Description was changed from

==========
Add support for DateTimeFormat.formatToParts

Spec discussion:  https://github.com/tc39/ecma402/issues/30

It's in stage 4 and Firefox has already implemented it.

For now, it's added to HARMONY_IN_PROGRESS bucket behind
'--datetime-format-to-parts' flag.

BUG=v8:5244
TEST=intl/date-format/date-format-to-parts.js
TEST=test262/intl402/DateTimeFormat/prototype/formatToParts/*
==========

to

==========
Add support for DateTimeFormat.formatToParts

Spec discussion:  https://github.com/tc39/ecma402/issues/30

It's in stage 4 and Firefox has already implemented it.

For now, it's added to HARMONY_IN_PROGRESS bucket behind
'--datetime-format-to-parts' flag.

BUG=v8:5244
TEST=intl/date-format/date-format-to-parts.js
TEST=test262/intl402/DateTimeFormat/prototype/formatToParts/*

Committed: https://crrev.com/a3db819c9e438b3e735a7c9ca659c540ddc003bc
Cr-Commit-Position: refs/heads/master@{#39225}
==========

[commit-bot: I haz the power, 2016-09-06 22:57:19]

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