Bug 150094 – [ES6] Implement String.prototype.normalize

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

[Yusuke Suzuki]

By leveraging ICU, implement String.prototype.normalize to complete ES6 String extras.

[Yusuke Suzuki]

http://userguide.icu-project.org/transforms/normalization

[Yusuke Suzuki]

Created attachment 263056 [details]
Patch

Just implemented. We need to add tests

[Yusuke Suzuki]

Created attachment 263254 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 263254 [details]
Patch

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

Added comment.

> Source/JavaScriptCore/runtime/StringPrototype.cpp:1804
> +        return throwTypeError(exec);

Because this depends on ICU, I took the conservative implementation; ICU API can raise errors any time.
But practically, it does not raise any error here I think.

[Build Bot]

Comment on attachment 263254 [details]
Patch

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

New failing tests:
js/Object-getOwnPropertyNames.html

[Build Bot]

Created attachment 263255 [details]
Archive of layout-test-results from ews102 for mac-mavericks

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

[Build Bot]

Comment on attachment 263254 [details]
Patch

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

New failing tests:
js/Object-getOwnPropertyNames.html

[Build Bot]

Created attachment 263257 [details]
Archive of layout-test-results from ews105 for mac-mavericks-wk2

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

[Yusuke Suzuki]

Created attachment 263259 [details]
Patch

[Yusuke Suzuki]

Created attachment 263260 [details]
Patch

[Geoffrey Garen]

Comment on attachment 263260 [details]
Patch

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

r=me

> Source/JavaScriptCore/runtime/StringPrototype.cpp:1802
> +        // The behavior is not specified when normalize is failed.

is failed => fails

> Source/JavaScriptCore/runtime/StringPrototype.cpp:1803
> +        // Now we throw a type erorr since it seems that the contents of the string is invalid.

is invalid => are invalid

> Source/JavaScriptCore/runtime/StringPrototype.cpp:1815
> +    status = U_ZERO_ERROR;
> +    unorm_normalize(source, sourceLength, form, 0, buffer, normalizedStringLength, &status);
> +    if (U_FAILURE(status))
> +        return throwTypeError(exec);

Does normalization ever grow a string? If not, I'd suggest allocating a buffer the size of the existing string, and calling norm_normalize only once.

> Source/JavaScriptCore/runtime/StringPrototype.cpp:1830
> +    // The argument is not provided or is undefined.

// Verify that the argument is provided and is not undefined.

> Source/JavaScriptCore/runtime/StringPrototype.cpp:1843
> +        if (formString == String(ASCIILiteral("NFC")))
> +            form = UNORM_NFC;
> +        else if (formString == String(ASCIILiteral("NFD")))
> +            form = UNORM_NFD;
> +        else if (formString == String(ASCIILiteral("NFKC")))
> +            form = UNORM_NFKC;
> +        else if (formString == String(ASCIILiteral("NFKD")))
> +            form = UNORM_NFKD;

This is a lot of StringImpl allocation. Can we use char* to avoid allocation?

[Yusuke Suzuki]

Comment on attachment 263260 [details]
Patch

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

Thanks!

>> Source/JavaScriptCore/runtime/StringPrototype.cpp:1802
>> +        // The behavior is not specified when normalize is failed.
> 
> is failed => fails

Fixed.

>> Source/JavaScriptCore/runtime/StringPrototype.cpp:1803
>> +        // Now we throw a type erorr since it seems that the contents of the string is invalid.
> 
> is invalid => are invalid

Fixed.

>> Source/JavaScriptCore/runtime/StringPrototype.cpp:1815
>> +        return throwTypeError(exec);
> 
> Does normalization ever grow a string? If not, I'd suggest allocating a buffer the size of the existing string, and calling norm_normalize only once.

Ah, normalization can grow a string. The actual example is included in the test case.
For example, in NFKC, "\uFDFA" will be normalized to "\u0635\u0644\u0649\u0020\u0627\u0644\u0644\u0647\u0020\u0639\u0644\u064a\u0647\u0020\u0648\u0633\u0644\u0645" !!

>> Source/JavaScriptCore/runtime/StringPrototype.cpp:1830
>> +    // The argument is not provided or is undefined.
> 
> // Verify that the argument is provided and is not undefined.

Thanks. Fixed.

>> Source/JavaScriptCore/runtime/StringPrototype.cpp:1843
>> +            form = UNORM_NFKD;
> 
> This is a lot of StringImpl allocation. Can we use char* to avoid allocation?

Ah, oops. After seeing ASCIILiteral, WTFString and StringImpl implementation, it seems that `formString == "NFC"` is more efficient :)
Fixed.

[Yusuke Suzuki]

Committed r191235: <http://trac.webkit.org/changeset/191235>