Bug 169458 – [INTL] Implement Intl.DateTimeFormat.prototype.formatToParts

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Andy VanWagoner]

[INTL] Implement Intl.DateTimeFormat.prototype.formatToParts

[Andy VanWagoner]

Created attachment 304019 [details]
Patch

[Andy VanWagoner]

https://tc39.github.io/ecma402/#sec-Intl.DateTimeFormat.prototype.formatToParts

[Andy VanWagoner]

Created attachment 304020 [details]
Patch

[Build Bot]

Comment on attachment 304020 [details]
Patch

Attachment 304020 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/3280137

New failing tests:
jsc-layout-tests.yaml/js/script-tests/intl-datetimeformat.js.layout
jsc-layout-tests.yaml/js/script-tests/intl-datetimeformat.js.layout-no-ftl
jsc-layout-tests.yaml/js/script-tests/intl-datetimeformat.js.layout-ftl-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/intl-datetimeformat.js.layout-ftl-no-cjit
jsc-layout-tests.yaml/js/script-tests/intl-datetimeformat.js.layout-no-cjit
jsc-layout-tests.yaml/js/script-tests/intl-datetimeformat.js.layout-no-llint
jsc-layout-tests.yaml/js/script-tests/intl-datetimeformat.js.layout-dfg-eager-no-cjit

[Build Bot]

Comment on attachment 304020 [details]
Patch

Attachment 304020 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3280297

New failing tests:
js/intl-datetimeformat.html

[Build Bot]

Created attachment 304025 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 304020 [details]
Patch

Attachment 304020 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3280298

New failing tests:
js/intl-datetimeformat.html

[Build Bot]

Created attachment 304026 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Andy VanWagoner]

Created attachment 304027 [details]
Patch

[Build Bot]

Comment on attachment 304027 [details]
Patch

Attachment 304027 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/3280579

New failing tests:
jsc-layout-tests.yaml/js/script-tests/intl-datetimeformat.js.layout
jsc-layout-tests.yaml/js/script-tests/intl-datetimeformat.js.layout-no-ftl
jsc-layout-tests.yaml/js/script-tests/intl-datetimeformat.js.layout-ftl-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/intl-datetimeformat.js.layout-ftl-no-cjit
jsc-layout-tests.yaml/js/script-tests/intl-datetimeformat.js.layout-no-cjit
jsc-layout-tests.yaml/js/script-tests/intl-datetimeformat.js.layout-no-llint
jsc-layout-tests.yaml/js/script-tests/intl-datetimeformat.js.layout-dfg-eager-no-cjit

[Build Bot]

Comment on attachment 304027 [details]
Patch

Attachment 304027 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3280553

New failing tests:
js/intl-datetimeformat.html

[Build Bot]

Created attachment 304030 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 304027 [details]
Patch

Attachment 304027 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3280634

New failing tests:
js/intl-datetimeformat.html

[Build Bot]

Created attachment 304031 [details]
Archive of layout-test-results from ews114 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 304027 [details]
Patch

Attachment 304027 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3280691

New failing tests:
js/intl-datetimeformat.html

[Build Bot]

Created attachment 304032 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Andy VanWagoner]

Created attachment 304095 [details]
Patch

[JF Bastien]

Comment on attachment 304095 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=304095&action=review

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:962
> +        return throwRangeError(&exec, scope, ASCIILiteral("date value is not finite in DateTimeFormat formatToParts()"));

Can you test this?

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:965
> +    UFieldPositionIterator* fields = ufieldpositer_open(&status);

The early-returns below don't close the UFieldPositionIterator. It's only closed on line 1015 along the normal return path. Could you make this automatic with some RAII magic?

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:967
> +        return throwTypeError(&exec, scope, ASCIILiteral("failed to format date value"));

This is an ICU-internal error, right? It would be nice to differentiate it from the other error below with the same string.

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:978
> +        return throwTypeError(&exec, scope, ASCIILiteral("failed to format date value"));

Can you test this as well?

[JF Bastien]

So it looks like we have 4 copies of the ICU headers, and from the readme in them they're only there to build on some really old MacOS which didn't provide the ICU headers.

Would it be possible to just... get rid of those 4 copies?

Barring that, we should probably update all 4 to the same version.

[Andy VanWagoner]

(In reply to JF Bastien from comment #18)
> Comment on attachment 304095 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=304095&action=review
> 
> > Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:962
> > +        return throwRangeError(&exec, scope, ASCIILiteral("date value is not finite in DateTimeFormat formatToParts()"));
> 
> Can you test this?

Will do.

> 
> > Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:965
> > +    UFieldPositionIterator* fields = ufieldpositer_open(&status);
> 
> The early-returns below don't close the UFieldPositionIterator. It's only
> closed on line 1015 along the normal return path. Could you make this
> automatic with some RAII magic?
> 

I'll add a unique_ptr. I had wondered before what that did before, and now I get it.

> > Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:967
> > +        return throwTypeError(&exec, scope, ASCIILiteral("failed to format date value"));
> 
> This is an ICU-internal error, right? It would be nice to differentiate it
> from the other error below with the same string.

Will do. I was worried about exposing implementation details in exceptions, but I can totally see how having a unique message for different cases makes troubleshooting easier if a bug ever comes in.

> 
> > Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:978
> > +        return throwTypeError(&exec, scope, ASCIILiteral("failed to format date value"));
> 
> Can you test this as well?

I'm not sure I know what case might cause ICU to fail. If you have a suggestion, I'd love to cover this case with a test.

[Andy VanWagoner]

Created attachment 307076 [details]
Patch

[Andy VanWagoner]

(In reply to JF Bastien from comment #19)
> So it looks like we have 4 copies of the ICU headers, and from the readme in
> them they're only there to build on some really old MacOS which didn't
> provide the ICU headers.
> 
> Would it be possible to just... get rid of those 4 copies?
> 
> Barring that, we should probably update all 4 to the same version.

I think it would be great to remove the ICU headers. As best I can tell from the project files we probably don't build for earlier than 10.10 at this point.

Can we look at making a sweeping change like that (removing or updating all ICU headers) in a separate patch?

[JF Bastien]

Comment on attachment 307076 [details]
Patch

Sorry for the delay, this scrolled off my inbox :(

(In reply to Andy VanWagoner from comment #22)
> (In reply to JF Bastien from comment #19)
> > So it looks like we have 4 copies of the ICU headers, and from the readme in
> > them they're only there to build on some really old MacOS which didn't
> > provide the ICU headers.
> > 
> > Would it be possible to just... get rid of those 4 copies?
> > 
> > Barring that, we should probably update all 4 to the same version.
> 
> I think it would be great to remove the ICU headers. As best I can tell from
> the project files we probably don't build for earlier than 10.10 at this
> point.
> 
> Can we look at making a sweeping change like that (removing or updating all
> ICU headers) in a separate patch?

Yes, this sounds good. Can you file a bug for it?

[Andy VanWagoner]

(In reply to JF Bastien from comment #23)
> Comment on attachment 307076 [details]
> Patch
> 
> Sorry for the delay, this scrolled off my inbox :(
> 
> (In reply to Andy VanWagoner from comment #22)
> > (In reply to JF Bastien from comment #19)
> > > So it looks like we have 4 copies of the ICU headers, and from the readme in
> > > them they're only there to build on some really old MacOS which didn't
> > > provide the ICU headers.
> > > 
> > > Would it be possible to just... get rid of those 4 copies?
> > > 
> > > Barring that, we should probably update all 4 to the same version.
> > 
> > I think it would be great to remove the ICU headers. As best I can tell from
> > the project files we probably don't build for earlier than 10.10 at this
> > point.
> > 
> > Can we look at making a sweeping change like that (removing or updating all
> > ICU headers) in a separate patch?
> 
> Yes, this sounds good. Can you file a bug for it?

Done. https://bugs.webkit.org/show_bug.cgi?id=170997

Can you also add cq+?

[JF Bastien]

Comment on attachment 307076 [details]
Patch

cq+

[WebKit Commit Bot]

Comment on attachment 307076 [details]
Patch

Clearing flags on attachment: 307076

Committed r215520: <http://trac.webkit.org/changeset/215520>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Carlos Alberto Lopez Perez]

(In reply to WebKit Commit Bot from comment #26)
> Comment on attachment 307076 [details]
> Patch
> 
> Clearing flags on attachment: 307076
> 
> Committed r215520: <http://trac.webkit.org/changeset/215520>

This has broken the build for our baseline minimum dependencies bot on Debian 8:

https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release%20Debian%20Stable%20%28Build%29/builds/945

Debian 8 has ICU 52.1 which don't has the header unicode/ufieldpositer.h

Seems that to use this header and its features, at least ICU 55 is needed.

I wonder if there is an easy way to ifdef this code, so at least it builds on ICU < 55 ?

[JF Bastien]

(In reply to Carlos Alberto Lopez Perez from comment #28)
> (In reply to WebKit Commit Bot from comment #26)
> > Comment on attachment 307076 [details]
> > Patch
> > 
> > Clearing flags on attachment: 307076
> > 
> > Committed r215520: <http://trac.webkit.org/changeset/215520>
> 
> This has broken the build for our baseline minimum dependencies bot on
> Debian 8:
> 
> https://build.webkit.org/builders/GTK%20Linux%2064-
> bit%20Release%20Debian%20Stable%20%28Build%29/builds/945
> 
> Debian 8 has ICU 52.1 which don't has the header unicode/ufieldpositer.h
> 
> Seems that to use this header and its features, at least ICU 55 is needed.
> 
> I wonder if there is an easy way to ifdef this code, so at least it builds
> on ICU < 55 ?

Rolled out in https://trac.webkit.org/changeset/215526/webkit

Debian has the header right? So the code could indeed detect this statically (__has_include or something?)

Otherwise we'd need a runtime detection :(

[Carlos Alberto Lopez Perez]

(In reply to JF Bastien from comment #29)
> Debian has the header right? So the code could indeed detect this statically
> (__has_include or something?)
> 

No, it don't has the header unicode/ufieldpositer.h

This header is available on ICU >= 55. Debian 8 still ships with ICU 52.


> Otherwise we'd need a runtime detection :(

I think is enough with some ifdef at build time, so the code that depends on unicode/ufieldpositer.h only gets built when that header is available on the system (aka when ICU version is >= 55)

[JF Bastien]

(In reply to Carlos Alberto Lopez Perez from comment #30)
> (In reply to JF Bastien from comment #29)
> > Debian has the header right? So the code could indeed detect this statically
> > (__has_include or something?)
> > 
> 
> No, it don't has the header unicode/ufieldpositer.h
> 
> This header is available on ICU >= 55. Debian 8 still ships with ICU 52.

Sorry what I meant was: Debian has ICU headers *in general* and doesn't use WebKit's local copies? If that's the case then we don't need anything special about Debian, all we need is to detect the presence of that header.

Whereas when a build uses the WebKit version of the headers they're obviously always present irrespective of the ICU library making the corresponding function available or not. In that case we'd need to weak-link or something like that.


> > Otherwise we'd need a runtime detection :(
> 
> I think is enough with some ifdef at build time, so the code that depends on
> unicode/ufieldpositer.h only gets built when that header is available on the
> system (aka when ICU version is >= 55)

[Michael Catanzaro]

(In reply to JF Bastien from comment #31)
> Sorry what I meant was: Debian has ICU headers *in general* and doesn't use
> WebKit's local copies? If that's the case then we don't need anything
> special about Debian, all we need is to detect the presence of that header.

Of course. If WebKit's bundled ICU headers are ever used in Linux builds, then that would be a serious bug, because WebKit uses the system copy of ICU and does not bundle ICU itself. (It's really unclear to me why the bundled headers are needed on Mac, since I don't believe you bundle ICU either.)

Anyway, solution here should be something like:

#include <unicode/uversion.h>

#if U_ICU_VERSION_MAJOR_NUM >= 55
#include <unicode/ufieldpositer.h>
#else
// ...
#endif

[Carlos Alberto Lopez Perez]

(In reply to JF Bastien from comment #31)
> (In reply to Carlos Alberto Lopez Perez from comment #30)
> > (In reply to JF Bastien from comment #29)
> > > Debian has the header right? So the code could indeed detect this statically
> > > (__has_include or something?)
> > > 
> > 
> > No, it don't has the header unicode/ufieldpositer.h
> > 
> > This header is available on ICU >= 55. Debian 8 still ships with ICU 52.
> 
> Sorry what I meant was: Debian has ICU headers *in general* and doesn't use
> WebKit's local copies? If that's the case then we don't need anything
> special about Debian, all we need is to detect the presence of that header.
> 
> Whereas when a build uses the WebKit version of the headers they're
> obviously always present irrespective of the ICU library making the
> corresponding function available or not. In that case we'd need to weak-link
> or something like that.
> 
> 
> > > Otherwise we'd need a runtime detection :(
> > 
> > I think is enough with some ifdef at build time, so the code that depends on
> > unicode/ufieldpositer.h only gets built when that header is available on the
> > system (aka when ICU version is >= 55)

(In reply to Michael Catanzaro from comment #32)
> (In reply to JF Bastien from comment #31)
> > Sorry what I meant was: Debian has ICU headers *in general* and doesn't use
> > WebKit's local copies? If that's the case then we don't need anything
> > special about Debian, all we need is to detect the presence of that header.
> 
> Of course. If WebKit's bundled ICU headers are ever used in Linux builds,
> then that would be a serious bug, because WebKit uses the system copy of ICU
> and does not bundle ICU itself. (It's really unclear to me why the bundled
> headers are needed on Mac, since I don't believe you bundle ICU either.)
> 

Right. We always use the icu provided by the system.

This are the headers available on debian 8 for icu: https://packages.debian.org/jessie/amd64/libicu-dev/filelist

[Andy VanWagoner]

(In reply to Michael Catanzaro from comment #32)
> (In reply to JF Bastien from comment #31)
> > Sorry what I meant was: Debian has ICU headers *in general* and doesn't use
> > WebKit's local copies? If that's the case then we don't need anything
> > special about Debian, all we need is to detect the presence of that header.
> 
> Of course. If WebKit's bundled ICU headers are ever used in Linux builds,
> then that would be a serious bug, because WebKit uses the system copy of ICU
> and does not bundle ICU itself. (It's really unclear to me why the bundled
> headers are needed on Mac, since I don't believe you bundle ICU either.)
> 
> Anyway, solution here should be something like:
> 
> #include <unicode/uversion.h>
> 
> #if U_ICU_VERSION_MAJOR_NUM >= 55
> #include <unicode/ufieldpositer.h>
> #else
> // ...
> #endif

I can add this. Do tests need to be conditional too?

[Carlos Alberto Lopez Perez]

(In reply to Andy VanWagoner from comment #34)
> (In reply to Michael Catanzaro from comment #32)
> > (In reply to JF Bastien from comment #31)
> > > Sorry what I meant was: Debian has ICU headers *in general* and doesn't use
> > > WebKit's local copies? If that's the case then we don't need anything
> > > special about Debian, all we need is to detect the presence of that header.
> > 
> > Of course. If WebKit's bundled ICU headers are ever used in Linux builds,
> > then that would be a serious bug, because WebKit uses the system copy of ICU
> > and does not bundle ICU itself. (It's really unclear to me why the bundled
> > headers are needed on Mac, since I don't believe you bundle ICU either.)
> > 
> > Anyway, solution here should be something like:
> > 
> > #include <unicode/uversion.h>
> > 
> > #if U_ICU_VERSION_MAJOR_NUM >= 55
> > #include <unicode/ufieldpositer.h>
> > #else
> > // ...
> > #endif
> 
> I can add this. Do tests need to be conditional too?

No. Its fine if the tests fail or are broken on systems with ICU <= 55.

[Carlos Alberto Lopez Perez]

(In reply to Carlos Alberto Lopez Perez from comment #35)
> (In reply to Andy VanWagoner from comment #34)
> > (In reply to Michael Catanzaro from comment #32)
> > > (In reply to JF Bastien from comment #31)
> > > > Sorry what I meant was: Debian has ICU headers *in general* and doesn't use
> > > > WebKit's local copies? If that's the case then we don't need anything
> > > > special about Debian, all we need is to detect the presence of that header.
> > > 
> > > Of course. If WebKit's bundled ICU headers are ever used in Linux builds,
> > > then that would be a serious bug, because WebKit uses the system copy of ICU
> > > and does not bundle ICU itself. (It's really unclear to me why the bundled
> > > headers are needed on Mac, since I don't believe you bundle ICU either.)
> > > 
> > > Anyway, solution here should be something like:
> > > 
> > > #include <unicode/uversion.h>
> > > 
> > > #if U_ICU_VERSION_MAJOR_NUM >= 55
> > > #include <unicode/ufieldpositer.h>
> > > #else
> > > // ...
> > > #endif
> > 
> > I can add this. Do tests need to be conditional too?
> 
> No. Its fine if the tests fail or are broken on systems with ICU <= 55.

                 ^^^^^     I mean: on systems with ICU < 55

[Andy VanWagoner]

Created attachment 307547 [details]
Patch

[Andy VanWagoner]

Comment on attachment 307547 [details]
Patch

Now with "#if U_ICU_VERSION_MAJOR_NUM >= 55"™

[JF Bastien]

Comment on attachment 307547 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=307547&action=review

Two minor things to address, then LGTM.

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:45
> +#if U_ICU_VERSION_MAJOR_NUM >= 55

Could you move the version 55 check to a header, and conditionally define a macro such as:

#if U_ICU_VERSION_MAJOR_NUM >= 55
#define JSC_ICU_HAS_UFIELDPOSITER 1
#endif

So after that you just use that macro, and only ever check for version 55 in a single spot.

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:957
> +    case UDAT_TIME_SEPARATOR_FIELD:

It looks like UDAT_TIME_SEPARATOR_FIELD is new to ICU 55, so this wouldn't compile (and partTypeString isn't compiled conditionally)?

[Carlos Alberto Lopez Perez]

This still fails to build with this error:

../../Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp: In static member function 'static const char* JSC::IntlDateTimeFormat::partTypeString(UDateFormatField)':
../../Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:956:10: error: 'UDAT_RELATED_YEAR_FIELD' was not declared in this scope
../../Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:957:10: error: 'UDAT_TIME_SEPARATOR_FIELD' was not declared in this scope

The following patch will fix the build error:

--- a/Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp
+++ b/Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp
@@ -953,8 +953,10 @@ const char* IntlDateTimeFormat::partTypeString(UDateFormatField field)
     case UDAT_MILLISECONDS_IN_DAY_FIELD:
     case UDAT_QUARTER_FIELD:
     case UDAT_STANDALONE_QUARTER_FIELD:
+#if U_ICU_VERSION_MAJOR_NUM >= 55
     case UDAT_RELATED_YEAR_FIELD:
     case UDAT_TIME_SEPARATOR_FIELD:
+#endif
     case UDAT_FIELD_COUNT:
         return "literal";
     }

[Andy VanWagoner]

Created attachment 307593 [details]
Patch

[JF Bastien]

Comment on attachment 307593 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=307593&action=review

Thanks! I noticed two small things, sorry for missing them earlier. I'll CQ after you fix.

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.h:119
> +    WriteBarrier<JSBoundFunction> m_boundFormatToParts;

I just noticed that this field is unused?

> Source/JavaScriptCore/runtime/IntlDateTimeFormatPrototype.cpp:87
> +#endif

Can you add:
#else
  UNUSED_PARAM(globalObject);

[Andy VanWagoner]

Created attachment 307663 [details]
Patch

[JF Bastien]

Comment on attachment 307663 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 307663 [details]
Patch

Clearing flags on attachment: 307663

Committed r215616: <http://trac.webkit.org/changeset/215616>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Michael Catanzaro]

Minor regression in bug #171241