[webkit-reviews] review granted: [Bug 169458] [INTL] Implement Intl.DateTimeFormat.prototype.formatToParts : [Attachment 307547] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Apr 19 22:39:22 PDT 2017
JF Bastien <jfbastien at apple.com> has granted Andy VanWagoner
<thetalecrafter at gmail.com>'s request for review:
Bug 169458: [INTL] Implement Intl.DateTimeFormat.prototype.formatToParts
https://bugs.webkit.org/show_bug.cgi?id=169458
Attachment 307547: Patch
https://bugs.webkit.org/attachment.cgi?id=307547&action=review
--- Comment #39 from JF Bastien <jfbastien at apple.com> ---
Comment on attachment 307547
--> https://bugs.webkit.org/attachment.cgi?id=307547
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)?
More information about the webkit-reviews
mailing list