From 46e06ad8fdaff580931d31c812157a08c565e7da Mon Sep 17 00:00:00 2001 From: Frank Tang Date: Wed, 16 Sep 2020 08:03:45 -0700 Subject: [PATCH] Fix locale of Intl.Collator..resolvedOptions Bug: v8:7481, v8:9084, v8:8664 Change-Id: Iccbf78bf11a4e8ca5d105772fa5f654fbe6542cd Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2410791 Commit-Queue: Frank Tang Reviewed-by: Jakob Kummerow Cr-Commit-Position: refs/heads/master@{#69951} --- src/objects/intl-objects.tq | 1 + src/objects/js-collator.cc | 29 ++++++++++++++++++++++++++-- test/intl/intl.status | 4 ---- test/intl/regress-9084.js | 10 ++++++++++ test/mjsunit/regress/regress-6288.js | 4 ++-- test/test262/test262.status | 7 ------- 6 files changed, 40 insertions(+), 15 deletions(-) create mode 100644 test/intl/regress-9084.js diff --git a/src/objects/intl-objects.tq b/src/objects/intl-objects.tq index 0868bc826c..88714f2bee 100644 --- a/src/objects/intl-objects.tq +++ b/src/objects/intl-objects.tq @@ -154,4 +154,5 @@ extern class JSV8BreakIterator extends JSObject { extern class JSCollator extends JSObject { icu_collator: Foreign; // Managed bound_compare: Undefined|JSFunction; + locale: String; } diff --git a/src/objects/js-collator.cc b/src/objects/js-collator.cc index b3b3c8fb55..fee19acfc8 100644 --- a/src/objects/js-collator.cc +++ b/src/objects/js-collator.cc @@ -196,8 +196,26 @@ Handle JSCollator::ResolvedOptions(Isolate* isolate, // [[Collation]] "collation" // [[Numeric]] "numeric" kn // [[CaseFirst]] "caseFirst" kf - CreateDataPropertyForOptions( - isolate, options, isolate->factory()->locale_string(), locale.c_str()); + + // If the collator return the locale differ from what got requested, we stored + // it in the collator->locale. Otherwise, we just use the one from the + // collator. + if (collator->locale().length() != 0) { + // Get the locale from collator->locale() since we know in some cases + // collator won't be able to return the requested one, such as zh_CN. + Handle locale_from_collator(collator->locale(), isolate); + Maybe maybe = JSReceiver::CreateDataProperty( + isolate, options, isolate->factory()->locale_string(), + locale_from_collator, Just(kDontThrow)); + DCHECK(maybe.FromJust()); + USE(maybe); + } else { + // Just return from the collator for most of the cases that we can recover + // from the collator. + CreateDataPropertyForOptions( + isolate, options, isolate->factory()->locale_string(), locale.c_str()); + } + CreateDataPropertyForOptions(isolate, options, isolate->factory()->usage_string(), usage); CreateDataPropertyForOptions( @@ -424,6 +442,9 @@ MaybeHandle JSCollator::New(Isolate* isolate, Handle map, } DCHECK(U_SUCCESS(status)); + icu::Locale collator_locale( + icu_collator->getLocale(ULOC_VALID_LOCALE, status)); + // 22. If relevantExtensionKeys contains "kn", then // a. Set collator.[[Numeric]] to ! SameValue(r.[[kn]], "true"). // @@ -524,8 +545,12 @@ MaybeHandle JSCollator::New(Isolate* isolate, Handle map, // Now all properties are ready, so we can allocate the result object. Handle collator = Handle::cast( isolate->factory()->NewFastOrSlowJSObjectFromMap(map)); + // We only need to do so if it is different from the collator would return. + Handle locale_str = isolate->factory()->NewStringFromAsciiChecked( + (collator_locale != icu_locale) ? r.locale.c_str() : ""); DisallowHeapAllocation no_gc; collator->set_icu_collator(*managed_collator); + collator->set_locale(*locale_str); // 29. Return collator. return collator; diff --git a/test/intl/intl.status b/test/intl/intl.status index 419292047d..ee54c92461 100644 --- a/test/intl/intl.status +++ b/test/intl/intl.status @@ -29,10 +29,6 @@ [ALWAYS, { # TODO(jochen): The following test is flaky. 'overrides/caching': [PASS, FAIL], - - # https://code.google.com/p/v8/issues/detail?id=7481 - 'collator/check-kf-option': [FAIL], - 'collator/check-kn-option': [FAIL], }], # ALWAYS ['variant == no_wasm_traps', { diff --git a/test/intl/regress-9084.js b/test/intl/regress-9084.js new file mode 100644 index 0000000000..2974d8b845 --- /dev/null +++ b/test/intl/regress-9084.js @@ -0,0 +1,10 @@ +// Copyright 2020 the V8 project authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +["zh", "zh-CN", "zh-Hans", "zh-Hans-CN"].forEach((l) => { + assertEquals( + l, + (new Intl.Collator( + l, {localeMatcher: "lookup"} )).resolvedOptions().locale); +}); diff --git a/test/mjsunit/regress/regress-6288.js b/test/mjsunit/regress/regress-6288.js index 5f550c31c8..1df666f230 100644 --- a/test/mjsunit/regress/regress-6288.js +++ b/test/mjsunit/regress/regress-6288.js @@ -5,9 +5,9 @@ // Environment Variables: LC_ALL=pt-BR.UTF8 // The data files packaged with d8 currently have Brazillian Portuguese -// DateTimeFormat but not Collation +// DateTimeFormat and Collation if (this.Intl) { - assertEquals('pt', Intl.Collator().resolvedOptions().locale); + assertEquals('pt-BR', Intl.Collator().resolvedOptions().locale); assertEquals('pt-BR', Intl.DateTimeFormat().resolvedOptions().locale); } diff --git a/test/test262/test262.status b/test/test262/test262.status index a40105af15..a577954c9b 100644 --- a/test/test262/test262.status +++ b/test/test262/test262.status @@ -62,10 +62,6 @@ # https://bugs.chromium.org/p/v8/issues/detail?id=4709 'language/expressions/assignment/fn-name-lhs-cover': [FAIL], - # https://bugs.chromium.org/p/v8/issues/detail?id=9084 - 'intl402/supportedLocalesOf-consistent-with-resolvedOptions': [FAIL], - 'intl402/fallback-locales-are-supported': [FAIL], - # https://code.google.com/p/v8/issues/detail?id=4251 'language/expressions/postfix-increment/S11.3.1_A5_T1': [FAIL], 'language/expressions/postfix-increment/S11.3.1_A5_T2': [FAIL], @@ -447,9 +443,6 @@ 'language/expressions/call/eval-spread-empty-leading': [FAIL], 'language/expressions/call/eval-spread-empty-trailing': [FAIL], - # https://bugs.chromium.org/p/v8/issues/detail?id=8664 - 'intl402/Collator/missing-unicode-ext-value-defaults-to-true': [FAIL], - # https://bugs.chromium.org/p/v8/issues/detail?id=7472 'intl402/NumberFormat/currency-digits': [FAIL],