Bug 210457 - [ECMA-402] Extension values should default to true, canonicalize without "-true"
Summary: [ECMA-402] Extension values should default to true, canonicalize without "-true"
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ross Kirsling
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-04-13 14:38 PDT by Ross Kirsling
Modified: 2020-04-15 16:34 PDT (History)
9 users (show)

See Also:


Attachments
Patch (4.20 KB, patch)
2020-04-13 15:03 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (12.63 KB, patch)
2020-04-13 15:31 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (12.64 KB, patch)
2020-04-15 13:08 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ross Kirsling 2020-04-13 14:38:44 PDT
[ECMA-402] Extension values should default to true, canonicalize without "-true"
Comment 1 Ross Kirsling 2020-04-13 15:03:34 PDT
Created attachment 396338 [details]
Patch
Comment 2 Ross Kirsling 2020-04-13 15:31:31 PDT
Created attachment 396345 [details]
Patch
Comment 3 Ross Kirsling 2020-04-15 12:59:18 PDT
Comment on attachment 396345 [details]
Patch

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

> JSTests/stress/intl-collator.js:85
> -shouldBe(testCollator(Intl.Collator('en-u-kn'), [{locale: 'en', numeric: true}]), true);
> -shouldBe(testCollator(Intl.Collator('en-u-kn-true'), [{locale: 'en-u-kn-true', numeric: true}]), true);
> +shouldBe(testCollator(Intl.Collator('en-u-kn'), [{locale: 'en-u-kn', numeric: true}]), true);
> +shouldBe(testCollator(Intl.Collator('en-u-kn-true'), [{locale: 'en-u-kn', numeric: true}]), true);

These two test cases are basically the change in a nutshell.
Comment 4 Ross Kirsling 2020-04-15 13:08:08 PDT
Created attachment 396564 [details]
Patch

Make ChangeLog clearer, fix string literal
Comment 5 Yusuke Suzuki 2020-04-15 13:44:33 PDT
Comment on attachment 396564 [details]
Patch

r=me
Comment 6 EWS 2020-04-15 14:18:42 PDT
Committed r260151: <https://trac.webkit.org/changeset/260151>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 396564 [details].
Comment 7 Radar WebKit Bug Importer 2020-04-15 14:19:14 PDT
<rdar://problem/61847887>
Comment 8 Darin Adler 2020-04-15 15:41:47 PDT
Comment on attachment 396564 [details]
Patch

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

> Source/JavaScriptCore/runtime/IntlObject.cpp:539
> +            if (lowercase != "true"_s) {

There is no benefit of using _s here, but it also does no harm. Generates exactly the same code, I believe. I suppose the rules for when ASCIILiteral is helpful are too confusing.

> Source/JavaScriptCore/runtime/IntlObject.cpp:541
> +                extension.append('-');
> +                extension.append(lowercase);

This one-liner is more efficient than two append calls.

    extension = makeString(extension, '-', lowercase);
Comment 9 Ross Kirsling 2020-04-15 15:44:16 PDT
(In reply to Darin Adler from comment #8)
> Comment on attachment 396564 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=396564&action=review
> 
> > Source/JavaScriptCore/runtime/IntlObject.cpp:539
> > +            if (lowercase != "true"_s) {
> 
> There is no benefit of using _s here, but it also does no harm. Generates
> exactly the same code, I believe. I suppose the rules for when ASCIILiteral
> is helpful are too confusing.

This one was Yusuke's suggestion. :)

> > Source/JavaScriptCore/runtime/IntlObject.cpp:541
> > +                extension.append('-');
> > +                extension.append(lowercase);
> 
> This one-liner is more efficient than two append calls.
> 
>     extension = makeString(extension, '-', lowercase);

Oh, that's good to know. I wasn't sure whether `foo = makeString(foo, ...)` was a good pattern or not. Will update.
Comment 10 Ross Kirsling 2020-04-15 16:34:52 PDT
(In reply to Ross Kirsling from comment #9)
> Oh, that's good to know. I wasn't sure whether `foo = makeString(foo, ...)`
> was a good pattern or not. Will update.

Committed r260161: <https://trac.webkit.org/changeset/260161>