Bug 185836 – [INTL] Language tags are not canonicalized

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Andy VanWagoner]

[INTL] Language tags are not canonicalized

[Andy VanWagoner]

Created attachment 340884 [details]
Naive

[Andy VanWagoner]

Comment on attachment 340884 [details]
Naive

There are multiple static constant maps. These are being naively created and destroyed in every pass. What is the best way to have these created once instead of each time? In debug mode the test262/intl402 tests take roughly 4x as long as they do without this patch.

This does not cover every possible case for canonicalization, and the mappings could also change over time, and probably should be generated from the iana subtag registry[1]. What's the preferred way to script that sort of thing?

This removes support for a few algorithmic numbering systems (like "roman" and "jpanfin") that were causing problems. Percent style was simply ignored, and currency would throw an error. Even with decimal style, roman numerals would get messed up when formatting large numbers. `(123456).toLocaleString('en-u-nu-roman') -> "ↈↂↂMMMCDLVI"`

[1] https://www.iana.org/assignments/language-subtag-registry/language-subtag-registry

[EWS Watchlist]

Comment on attachment 340884 [details]
Naive

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

New failing tests:
js/intl-datetimeformat.html

[EWS Watchlist]

Created attachment 340902 [details]
Archive of layout-test-results from ews101 for mac-sierra

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

[EWS Watchlist]

Comment on attachment 340884 [details]
Naive

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

New failing tests:
js/intl-datetimeformat.html

[EWS Watchlist]

Created attachment 340904 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

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

[EWS Watchlist]

Comment on attachment 340884 [details]
Naive

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

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

[EWS Watchlist]

Comment on attachment 340884 [details]
Naive

Attachment 340884 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/7757468

New failing tests:
js/intl-datetimeformat.html

[EWS Watchlist]

Created attachment 340911 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4

[EWS Watchlist]

Comment on attachment 340884 [details]
Naive

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

New failing tests:
js/intl-datetimeformat.html

[EWS Watchlist]

Created attachment 340912 [details]
Archive of layout-test-results from ews115 for mac-sierra

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

[Andy VanWagoner]

Created attachment 341106 [details]
Patch

[Andy VanWagoner]

Created attachment 341113 [details]
Script extracting info from registry

[Andy VanWagoner]

Created attachment 341131 [details]
Patch

[Andy VanWagoner]

Comment on attachment 341131 [details]
Patch

There are probably a few edge cases for canonicalization missing, but generates code from the language subtag registry, covers cases tested by test262/intl402, and generally does a way better job of canonicalizing than before.

If we find a more efficient way to do the lookups, we can modify the script to generate it, instead of manually updating several c++ functions.

[Andy VanWagoner]

Is there anything else this patch needs before it can be considered?

[Andy VanWagoner]

Created attachment 343739 [details]
Rebased

[EWS Watchlist]

Comment on attachment 343739 [details]
Rebased

Attachment 343739 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8367720

New failing tests:
http/tests/security/local-video-source-from-remote.html

[EWS Watchlist]

Created attachment 343802 [details]
Archive of layout-test-results from ews205 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews205  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Andy VanWagoner]

Created attachment 344285 [details]
Patch

[EWS Watchlist]

Comment on attachment 344285 [details]
Patch

Attachment 344285 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8436832

New failing tests:
http/tests/security/canvas-remote-read-remote-video-redirect.html

[EWS Watchlist]

Created attachment 344292 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Andy VanWagoner]

Created attachment 345573 [details]
Patch

[Keith Miller]

Comment on attachment 345573 [details]
Patch

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

r=me with some comments.

> Source/JavaScriptCore/Scripts/generateIntlCanonicalizeLanguage.py:113
> +        # file.write("#include <wtf/HashMap.h>\n")
> +        # file.write("#include <wtf/NeverDestroyed.h>\n")

Please remove.

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:679
> +    ucal_setGregorianChange(cal, -8.64E15, &status);

What's -8.64E15? Maybe it'd be better as a named const variable?

> Source/JavaScriptCore/runtime/IntlObject.cpp:327
> +                if (!times) {
> +                    if (intlPreferredExtlangTag(extlangLower) == langtag) {

Nit: Can you make this if (!times && intlPreferredExtlangTag(extlangLower) == langtag)

> Source/JavaScriptCore/ucd/language-subtag-registry.txt:1
> +File-Date: 2018-04-23

Does this file have a Copyright? Do we need to do anything there.

[Keith Miller]

Thanks again for taking the time to keep the patch up to date! Sorry about the delay in the review... :\

[Andy VanWagoner]

(In reply to Keith Miller from comment #24)
> Comment on attachment 345573 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=345573&action=review
> 
> r=me with some comments.
> 
> > Source/JavaScriptCore/Scripts/generateIntlCanonicalizeLanguage.py:113
> > +        # file.write("#include <wtf/HashMap.h>\n")
> > +        # file.write("#include <wtf/NeverDestroyed.h>\n")
> 
> Please remove.

Will Do.

> 
> > Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:679
> > +    ucal_setGregorianChange(cal, -8.64E15, &status);
> 
> What's -8.64E15? Maybe it'd be better as a named const variable?

It's the min ECMAScript time. A const for it sounds good.

> 
> > Source/JavaScriptCore/runtime/IntlObject.cpp:327
> > +                if (!times) {
> > +                    if (intlPreferredExtlangTag(extlangLower) == langtag) {
> 
> Nit: Can you make this if (!times && intlPreferredExtlangTag(extlangLower)
> == langtag)

Sure thing.

> 
> > Source/JavaScriptCore/ucd/language-subtag-registry.txt:1
> > +File-Date: 2018-04-23
> 
> Does this file have a Copyright? Do we need to do anything there.

This file is downloaded from https://www.iana.org/assignments/language-subtag-registry/language-subtag-registry I left it verbatim, but I can modify it if we need to. I figured downloading it in the script would be bad, relying on network connectivity for the build.

[Keith Miller]

Comment on attachment 345573 [details]
Patch

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

>>> Source/JavaScriptCore/ucd/language-subtag-registry.txt:1
>>> +File-Date: 2018-04-23
>> 
>> Does this file have a Copyright? Do we need to do anything there.
> 
> This file is downloaded from https://www.iana.org/assignments/language-subtag-registry/language-subtag-registry I left it verbatim, but I can modify it if we need to. I figured downloading it in the script would be bad, relying on network connectivity for the build.

Yeah, we definitely shouldn't download it each time. Based on http://trustee.ietf.org/license-info/IETF-TLP-5.pdf we need to include the Simplified BSD License at the top of the file. Can you do that before landing?

[EWS Watchlist]

Comment on attachment 345573 [details]
Patch

Attachment 345573 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8629263

New failing tests:
http/tests/security/canvas-remote-read-remote-video-localhost.html

[EWS Watchlist]

Created attachment 345599 [details]
Archive of layout-test-results from ews200 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Andy VanWagoner]

Created attachment 345600 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 345600 [details]
Patch

Clearing flags on attachment: 345600

Committed r234127: <https://trac.webkit.org/changeset/234127>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/42524024>

[Dawei Fenton (:realdawei)]

(In reply to WebKit Commit Bot from comment #31)
> Comment on attachment 345600 [details]
> Patch
> 
> Clearing flags on attachment: 345600
> 
> Committed r234127: <https://trac.webkit.org/changeset/234127>

seeing 262 errors on the bots after this revision

sample output:
https://build.webkit.org/builders/Apple%20Sierra%20Release%20Test262%20%28Tests%29/builds/3611/steps/test262-test/logs/stdio

FAIL test/intl402/Locale/constructor-non-iana-canon.js (default)
Full Output:
Exception: TypeError: undefined is not a constructor (evaluating 'new Intl.Locale(tag)')
global code@/Volumes/Data/slave/sierra-release-tests-test262/build/JSTests/test262/test/intl402/Locale/constructor-non-iana-canon.js:86:32

FAIL test/intl402/Locale/constructor-non-iana-canon.js (strict mode)
Full Output:
Exception: TypeError: undefined is not a constructor (evaluating 'new Intl.Locale(tag)')
global code@/Volumes/Data/slave/sierra-release-tests-test262/build/JSTests/test262/test/intl402/Locale/constructor-non-iana-canon.js:87:32

FAIL test/intl402/Locale/likely-subtags-grandfathered.js (default)
Full Output:
Exception: TypeError: undefined is not a constructor (evaluating 'new Intl.Locale(tag)')
global code@/Volumes/Data/slave/sierra-release-tests-test262/build/JSTests/test262/test/intl402/Locale/likely-subtags-grandfathered.js:98:32

FAIL test/intl402/Locale/likely-subtags-grandfathered.js (strict mode)
Full Output:
Exception: TypeError: undefined is not a constructor (evaluating 'new Intl.Locale(tag)')
global code@/Volumes/Data/slave/sierra-release-tests-test262/build/JSTests/test262/test/intl402/Locale/likely-subtags-grandfathered.js:99:32

[Andy VanWagoner]

(In reply to David Fenton (:realdawei) from comment #34)
> (In reply to WebKit Commit Bot from comment #31)
> > Comment on attachment 345600 [details]
> > Patch
> > 
> > Clearing flags on attachment: 345600
> > 
> > Committed r234127: <https://trac.webkit.org/changeset/234127>
> 
> seeing 262 errors on the bots after this revision
> 
> sample output:
> https://build.webkit.org/builders/
> Apple%20Sierra%20Release%20Test262%20%28Tests%29/builds/3611/steps/test262-
> test/logs/stdio
> 
> FAIL test/intl402/Locale/constructor-non-iana-canon.js (default)
> Full Output:
> Exception: TypeError: undefined is not a constructor (evaluating 'new
> Intl.Locale(tag)')
> global
> code@/Volumes/Data/slave/sierra-release-tests-test262/build/JSTests/test262/
> test/intl402/Locale/constructor-non-iana-canon.js:86:32
> 
> FAIL test/intl402/Locale/constructor-non-iana-canon.js (strict mode)
> Full Output:
> Exception: TypeError: undefined is not a constructor (evaluating 'new
> Intl.Locale(tag)')
> global
> code@/Volumes/Data/slave/sierra-release-tests-test262/build/JSTests/test262/
> test/intl402/Locale/constructor-non-iana-canon.js:87:32
> 
> FAIL test/intl402/Locale/likely-subtags-grandfathered.js (default)
> Full Output:
> Exception: TypeError: undefined is not a constructor (evaluating 'new
> Intl.Locale(tag)')
> global
> code@/Volumes/Data/slave/sierra-release-tests-test262/build/JSTests/test262/
> test/intl402/Locale/likely-subtags-grandfathered.js:98:32
> 
> FAIL test/intl402/Locale/likely-subtags-grandfathered.js (strict mode)
> Full Output:
> Exception: TypeError: undefined is not a constructor (evaluating 'new
> Intl.Locale(tag)')
> global
> code@/Volumes/Data/slave/sierra-release-tests-test262/build/JSTests/test262/
> test/intl402/Locale/likely-subtags-grandfathered.js:99:32

These appear to be newer tests related to `Intl.Locale` which we haven't implemented yet. They weren't accounted for in the expectations.yml file yet, and I didn't add them.

How does expectations.yml & the test262 tests usually get updated? If needed I can push up a patch that adds those as expected failures.

[Andy VanWagoner]

The patch for https://bugs.webkit.org/show_bug.cgi?id=187960 includes fixing the expectations for Intl.Locale tests that were changed by this.

[Dawei Fenton (:realdawei)]

(In reply to Andy VanWagoner from comment #36)
> The patch for https://bugs.webkit.org/show_bug.cgi?id=187960 includes fixing
> the expectations for Intl.Locale tests that were changed by this.

great. thanks!