Bug 206791 – Move singleton Intl string locales out of JSGlobalObject.

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

[Mark Lam]

We're creating an instance of these for each JSGlobalObject when they can be a global singleton since they are always initialized with the same intl data (barring a mid-flight change in intl settings, which we don't support even in the existing code).

Also fix a bug in StringImpl::createStaticStringImpl(): I forgot to set its hash value when I introduced it.  StaticStringImpls require that its hash code be set ahead of time, and cannot be mutated at runtime.

[Radar WebKit Bug Importer]

<rdar://problem/58889037>

[Andy VanWagoner]

That sounds good to me. I don’t know of any reason not to use a global singleton.

[Mark Lam]

Created attachment 388755 [details]
proposed patch.

[Mark Lam]

Comment on attachment 388755 [details]
proposed patch.

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

> Source/JavaScriptCore/runtime/IntlPluralRules.cpp:124
> -    const HashSet<String> availableLocales = globalObject->intlNumberFormatAvailableLocales();
> +    const HashSet<String>& availableLocales = intlNumberFormatAvailableLocales();

Andy, can you confirm that it is intentional to use intlNumberFormatAvailableLocales() instead of intlPluralRulesAvailableLocales() here?  I noticed that intlPluralRulesAvailableLocales() is called anywhere, which may me suspicious of this code.  Thought you might know better what is correct.

[Mark Lam]

(In reply to Mark Lam from comment #4)
> Andy, can you confirm that it is intentional to use
> intlNumberFormatAvailableLocales() instead of
> intlPluralRulesAvailableLocales() here?  I noticed that
> intlPluralRulesAvailableLocales() is called anywhere, which may me
> suspicious of this code.  Thought you might know better what is correct.

typo: I actually meant "intlPluralRulesAvailableLocales() is NOT called anywhere".

[Andy VanWagoner]

(In reply to Mark Lam from comment #5)
> (In reply to Mark Lam from comment #4)
> > Andy, can you confirm that it is intentional to use
> > intlNumberFormatAvailableLocales() instead of
> > intlPluralRulesAvailableLocales() here?  I noticed that
> > intlPluralRulesAvailableLocales() is called anywhere, which may me
> > suspicious of this code.  Thought you might know better what is correct.
> 
> typo: I actually meant "intlPluralRulesAvailableLocales() is NOT called
> anywhere".

I’ll double check.

[Andy VanWagoner]

The ECMA 402 spec calls for %PluralRules%.[[AvailableLocales]] to be passed into ResolveLocale during InitializePluralRules[0] and used in supportedLocalesOf[1]. I believe this is what intlPluralRulesAvailableLocales was created for.

However, there is no uplrules_getAvailable[2], so intlPluralRulesAvailableLocales was just using uloc_getAvailable (all available locales). It seems a better assumption that number formatting supporting locales would have plural rules support, than all known locales, and in IntlPluralRules::select, sometimes calls unum_formatDouble & unum_formatDouble too, so it seems appropriate to draw the available locales from unum_getAvailable.

IntlPluralRules::initializePluralRules[3] and IntlPluralRulesConstructorFuncSupportedLocalesOf[4] are already just using intlNumberFormatAvailableLocales, and I don't see a reason to duplicate that list into intlPluralRulesAvailableLocales. Let's just drop it entirely.

In JSC %PluralRules%.[[AvailableLocales]] will just be the same as %NumberFormat%.[[AvailableLocales]].

[0] https://tc39.es/ecma402/#sec-initializepluralrules
[1] https://tc39.es/ecma402/#sec-intl.pluralrules.supportedlocalesof
[2] https://unicode-org.github.io/icu-docs/apidoc/released/icu4c/upluralrules_8h.html
[3] https://trac.webkit.org/browser/webkit/trunk/Source/JavaScriptCore/runtime/IntlPluralRules.cpp#L124
[4] https://trac.webkit.org/browser/webkit/trunk/Source/JavaScriptCore/runtime/IntlPluralRulesConstructor.cpp#L120

[Andy VanWagoner]

We could have intlPluralRulesAvailableLocales be a reference to intlNumberFormatAvailableLocales, and use that in IntlPluralRules::initializePluralRules and IntlPluralRulesConstructorFuncSupportedLocalesOf.
That would remove the suspect use of intlNumberFormatAvailableLocales.

[Yusuke Suzuki]

Comment on attachment 388755 [details]
proposed patch.

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

r=me, nice.

> Source/JavaScriptCore/runtime/IntlObject.h:62
> +const HashSet<String>& intlNumberFormatAvailableLocales();

Define intlPluralRulesAvailableLocales.

>> Source/JavaScriptCore/runtime/IntlPluralRules.cpp:124
>> +    const HashSet<String>& availableLocales = intlNumberFormatAvailableLocales();
> 
> Andy, can you confirm that it is intentional to use intlNumberFormatAvailableLocales() instead of intlPluralRulesAvailableLocales() here?  I noticed that intlPluralRulesAvailableLocales() is called anywhere, which may me suspicious of this code.  Thought you might know better what is correct.

Thanks, Andy. Yeah, seems that,

defining intlPluralRulesAvailableLocales, which just calls intlNumberFormatAvailableLocales() internally.

Would be nice.

> Source/JavaScriptCore/runtime/IntlPluralRulesConstructor.cpp:121
> +    const HashSet<String>& availableLocales = intlNumberFormatAvailableLocales();

Yeah, use intlPluralRulesAvailableLocales, which should just call intlNumberFormatAvailableLocales() internally.

[Mark Lam]

Thanks for the reviews.  I've applied the requested changes.

Landed in r255120: <http://trac.webkit.org/r255120>.

[Mark Lam]

FYI, this patch also contains a fix for a bug in r255112:<http://trac.webkit.org/r255112> which was landed for https://bugs.webkit.org/show_bug.cgi?id=20677.

[Darin Adler]

Comment on attachment 388755 [details]
proposed patch.

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

> Source/WTF/wtf/text/StringImpl.cpp:287
> +    ASSERT(charactersAreAllASCII<LChar>(lcharCharacters, length));

The <LChar> here isn’t needed.

[Darin Adler]

Comment on attachment 388755 [details]
proposed patch.

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

> Source/WTF/ChangeLog:12
> +        Fix a bug in StringImpl::createStaticStringImpl(): I forgot to set its hash value
> +        when I introduced it.  StaticStringImpls require that its hash code be set ahead
> +        of time, and cannot be mutated at runtime.  See the comment in the definition of
> +        StaticStringImpl in StringImpl.h.

Do we have a test that shows this is correct now?

[Mark Lam]

(In reply to Darin Adler from comment #12)
> > Source/WTF/wtf/text/StringImpl.cpp:287
> > +    ASSERT(charactersAreAllASCII<LChar>(lcharCharacters, length));
> 
> The <LChar> here isn’t needed.

I copied this code over from StringImpl::createFromLiteral().  I'll fix it there too.

(In reply to Darin Adler from comment #13)
> > Source/WTF/ChangeLog:12
> > +        Fix a bug in StringImpl::createStaticStringImpl(): I forgot to set its hash value
> > +        when I introduced it.  StaticStringImpls require that its hash code be set ahead
> > +        of time, and cannot be mutated at runtime.  See the comment in the definition of
> > +        StaticStringImpl in StringImpl.h.
> 
> Do we have a test that shows this is correct now?

This was caught by existing JSC tests.  But I'll add some API tests.

Will do these in https://bugs.webkit.org/show_bug.cgi?id=206802.

[Mark Lam]

*** Bug 206869 has been marked as a duplicate of this bug. ***