Bug 169458

Summary: [INTL] Implement Intl.DateTimeFormat.prototype.formatToParts
Product: WebKit Reporter: Andy VanWagoner <thetalecrafter>
Component: JavaScriptCoreAssignee: Andy VanWagoner <thetalecrafter>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, bugs-noreply, buildbot, clopez, commit-queue, jfbastien, keith_miller, mark.lam, mcatanzaro, msaboff, rniwa, sbarati
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=170997
Bug Depends on:    
Bug Blocks: 170997    
Attachments:
Description Flags
Patch
none
Patch
none
Archive of layout-test-results from ews100 for mac-elcapitan
none
Archive of layout-test-results from ews104 for mac-elcapitan-wk2
none
Patch
none
Archive of layout-test-results from ews100 for mac-elcapitan
none
Archive of layout-test-results from ews114 for mac-elcapitan
none
Archive of layout-test-results from ews105 for mac-elcapitan-wk2
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Andy VanWagoner 2017-03-09 18:31:19 PST
[INTL] Implement Intl.DateTimeFormat.prototype.formatToParts
Comment 1 Andy VanWagoner 2017-03-09 18:41:19 PST
Created attachment 304019 [details]
Patch
Comment 3 Andy VanWagoner 2017-03-09 18:53:01 PST
Created attachment 304020 [details]
Patch
Comment 4 Build Bot 2017-03-09 19:30:52 PST
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
Comment 5 Build Bot 2017-03-09 19:58:24 PST
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
Comment 6 Build Bot 2017-03-09 19:58:27 PST
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
Comment 7 Build Bot 2017-03-09 20:00:21 PST
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
Comment 8 Build Bot 2017-03-09 20:00:24 PST
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
Comment 9 Andy VanWagoner 2017-03-09 20:02:10 PST
Created attachment 304027 [details]
Patch
Comment 10 Build Bot 2017-03-09 20:39:05 PST
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
Comment 11 Build Bot 2017-03-09 20:40:47 PST
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
Comment 12 Build Bot 2017-03-09 20:40:50 PST
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
Comment 13 Build Bot 2017-03-09 21:02:46 PST
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
Comment 14 Build Bot 2017-03-09 21:02:49 PST
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
Comment 15 Build Bot 2017-03-09 21:10:51 PST
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
Comment 16 Build Bot 2017-03-09 21:10:56 PST
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
Comment 17 Andy VanWagoner 2017-03-10 16:20:17 PST
Created attachment 304095 [details]
Patch
Comment 18 JF Bastien 2017-04-13 10:55:06 PDT
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?
Comment 19 JF Bastien 2017-04-13 14:49:17 PDT
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.
Comment 20 Andy VanWagoner 2017-04-13 17:54:05 PDT
(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.
Comment 21 Andy VanWagoner 2017-04-13 20:01:05 PDT
Created attachment 307076 [details]
Patch
Comment 22 Andy VanWagoner 2017-04-13 20:09:20 PDT
(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?
Comment 23 JF Bastien 2017-04-19 09:29:32 PDT
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?
Comment 24 Andy VanWagoner 2017-04-19 09:44:01 PDT
(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+?
Comment 25 JF Bastien 2017-04-19 10:37:16 PDT
Comment on attachment 307076 [details]
Patch

cq+
Comment 26 WebKit Commit Bot 2017-04-19 11:08:07 PDT
Comment on attachment 307076 [details]
Patch

Clearing flags on attachment: 307076

Committed r215520: <http://trac.webkit.org/changeset/215520>
Comment 27 WebKit Commit Bot 2017-04-19 11:08:09 PDT
All reviewed patches have been landed.  Closing bug.
Comment 28 Carlos Alberto Lopez Perez 2017-04-19 12:59:58 PDT
(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 ?
Comment 29 JF Bastien 2017-04-19 13:05:31 PDT
(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 :(
Comment 30 Carlos Alberto Lopez Perez 2017-04-19 13:51:47 PDT
(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)
Comment 31 JF Bastien 2017-04-19 13:57:22 PDT
(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)
Comment 32 Michael Catanzaro 2017-04-19 14:29:17 PDT
(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
Comment 33 Carlos Alberto Lopez Perez 2017-04-19 14:35:17 PDT
(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
Comment 34 Andy VanWagoner 2017-04-19 14:51:30 PDT
(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?
Comment 35 Carlos Alberto Lopez Perez 2017-04-19 15:01:20 PDT
(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.
Comment 36 Carlos Alberto Lopez Perez 2017-04-19 15:02:07 PDT
(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
Comment 37 Andy VanWagoner 2017-04-19 20:42:25 PDT
Created attachment 307547 [details]
Patch
Comment 38 Andy VanWagoner 2017-04-19 20:43:59 PDT
Comment on attachment 307547 [details]
Patch

Now with "#if U_ICU_VERSION_MAJOR_NUM >= 55"™
Comment 39 JF Bastien 2017-04-19 22:39:22 PDT
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)?
Comment 40 Carlos Alberto Lopez Perez 2017-04-20 01:27:30 PDT
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";
     }
Comment 41 Andy VanWagoner 2017-04-20 06:54:19 PDT
Created attachment 307593 [details]
Patch
Comment 42 JF Bastien 2017-04-20 09:19:42 PDT
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);
Comment 43 Andy VanWagoner 2017-04-20 16:58:36 PDT
Created attachment 307663 [details]
Patch
Comment 44 JF Bastien 2017-04-21 10:30:14 PDT
Comment on attachment 307663 [details]
Patch

r=me
Comment 45 WebKit Commit Bot 2017-04-21 10:58:36 PDT
Comment on attachment 307663 [details]
Patch

Clearing flags on attachment: 307663

Committed r215616: <http://trac.webkit.org/changeset/215616>
Comment 46 WebKit Commit Bot 2017-04-21 10:58:38 PDT
All reviewed patches have been landed.  Closing bug.
Comment 47 Michael Catanzaro 2017-04-24 12:32:40 PDT
Minor regression in bug #171241