Bug 194466 – [JSC] String.fromCharCode's slow path always generates 16bit string

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[JSC] String.fromCharCode's slow path always generates 16bit string

[Yusuke Suzuki]

Created attachment 361577 [details]
Patch

[Keith Miller]

Comment on attachment 361577 [details]
Patch

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

r=me with small fix.

> Source/JavaScriptCore/ChangeLog:10
> +        and even worse, taints ropes 16bit if 16bit string is included in the given rope. We find that acorn-wtb

Typo: We find => We found. Or if you want to take more credit: I found.

> Source/JavaScriptCore/ChangeLog:12
> +        16bit string. However, only few strings are actually 16bit strings. This patch attempts to make 8bit string

Typo: only few => only a few

> Source/JavaScriptCore/runtime/StringConstructor.cpp:91
> +        if (UNLIKELY(character & 0xFF00)) {

Can we use or create a named WTF function for this?

[Yusuke Suzuki]

Comment on attachment 361577 [details]
Patch

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

>> Source/JavaScriptCore/runtime/StringConstructor.cpp:91
>> +        if (UNLIKELY(character & 0xFF00)) {
> 
> Can we use or create a named WTF function for this?

Yeah, I added `is8BitCharacter(ch)`, since ASCIICType::isASCII(ch) is different from this definition. (It does not allow the upper most bit is set, since it is precisely out of ASCII range).

[Yusuke Suzuki]

Comment on attachment 361577 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:10
>> +        and even worse, taints ropes 16bit if 16bit string is included in the given rope. We find that acorn-wtb
> 
> Typo: We find => We found. Or if you want to take more credit: I found.

Fixed.

>> Source/JavaScriptCore/ChangeLog:12
>> +        16bit string. However, only few strings are actually 16bit strings. This patch attempts to make 8bit string
> 
> Typo: only few => only a few

Fixed.

[Keith Miller]

(In reply to Yusuke Suzuki from comment #3)
> Comment on attachment 361577 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=361577&action=review
> 
> >> Source/JavaScriptCore/runtime/StringConstructor.cpp:91
> >> +        if (UNLIKELY(character & 0xFF00)) {
> > 
> > Can we use or create a named WTF function for this?
> 
> Yeah, I added `is8BitCharacter(ch)`, since ASCIICType::isASCII(ch) is
> different from this definition. (It does not allow the upper most bit is
> set, since it is precisely out of ASCII range).

I think we should actually call it isLatin1. IIUC, that's the most precise term.

[EWS Watchlist]

Comment on attachment 361577 [details]
Patch

Attachment 361577 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11087027

New failing tests:
imported/w3c/web-platform-tests/webrtc/simplecall.https.html

[EWS Watchlist]

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

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

[Yusuke Suzuki]

Committed r241233: <https://trac.webkit.org/changeset/241233>

[Radar WebKit Bug Importer]

<rdar://problem/47938406>

[Darin Adler]

(In reply to Keith Miller from comment #5)
> I think we should actually call it isLatin1. IIUC, that's the most precise
> term.

There are isLatin1 functions in Lexer.cpp already. This patch adds a new one. Could we consolidate?

[Yusuke Suzuki]

(In reply to Darin Adler from comment #10)
> (In reply to Keith Miller from comment #5)
> > I think we should actually call it isLatin1. IIUC, that's the most precise
> > term.
> 
> There are isLatin1 functions in Lexer.cpp already. This patch adds a new
> one. Could we consolidate?

Oh, nice! I'll land it in a follow-up patch.

[Yusuke Suzuki]

Committed r241246: <https://trac.webkit.org/changeset/241246>