[INTL] Implement Intl.DateTimeFormat.prototype.formatToParts
Created attachment 304019 [details] Patch
https://tc39.github.io/ecma402/#sec-Intl.DateTimeFormat.prototype.formatToParts
Created attachment 304020 [details] Patch
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 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
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 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
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
Created attachment 304027 [details] Patch
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 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
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 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
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 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
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
Created attachment 304095 [details] Patch
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?
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.
(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.
Created attachment 307076 [details] Patch
(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 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?
(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 on attachment 307076 [details] Patch cq+
Comment on attachment 307076 [details] Patch Clearing flags on attachment: 307076 Committed r215520: <http://trac.webkit.org/changeset/215520>
All reviewed patches have been landed. Closing bug.
(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 ?
(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 :(
(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)
(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 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
(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
(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?
(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.
(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
Created attachment 307547 [details] Patch
Comment on attachment 307547 [details] Patch Now with "#if U_ICU_VERSION_MAJOR_NUM >= 55"™
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)?
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"; }
Created attachment 307593 [details] Patch
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);
Created attachment 307663 [details] Patch
Comment on attachment 307663 [details] Patch r=me
Comment on attachment 307663 [details] Patch Clearing flags on attachment: 307663 Committed r215616: <http://trac.webkit.org/changeset/215616>
Minor regression in bug #171241