Bug 99392 – Creating a String from an NSString should check for all 8 bit strings

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Michael Saboff]

In most cases NSString's that are used to create String's are all 8 bit (typically ASCII).  In these cases we should create 8 bit Strings.

[Michael Saboff]

Created attachment 168818 [details]
Patch

During simple testing, I never saw a 16 bit NSString.

[Darin Adler]

Comment on attachment 168818 [details]
Patch

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

Please look into putting the fast check for 8-bit characters in ASCIIFastPath.h instead of here.

> Source/WTF/wtf/text/StringImpl.cpp:1579
> +#if CPU(X86_64)
> +bool containsOnly8BitCharacters(const UChar* characters, unsigned length)

This looks like code that should be in the ASCIIFastPath.h, rather than in this source file. The same techniques used in charactersAreAllASCII are appropriate and that function is even better optimized than this code, I believe.

With a bit of work we could probably make the code for charactersAreAllASCII and charactersAreAll8Bit be generated from a single function template.

[Benjamin Poulain]

(In reply to comment #0)
> In most cases NSString's that are used to create String's are all 8 bit (typically ASCII).  In these cases we should create 8 bit Strings.

How common is this?
Can we fix those at the source, when the NSString/CFString are created? CoreFoundation also have shortcuts for 8bits, it would be nice if we could enjoy those.

> This looks like code that should be in the ASCIIFastPath.h, rather than in this source file. The same techniques used in charactersAreAllASCII are appropriate and that function is even better optimized than this code, I believe.
> 
> With a bit of work we could probably make the code for charactersAreAllASCII and charactersAreAll8Bit be generated from a single function template.

I would prefer a new header (Latin1FastPath.h?).
We are already abusing ASCIIFastPath.h with non-ASCII functions.

[Michael Saboff]

Created attachment 169039 [details]
Updated Patch

It appears that these NSStrings are used in network responses among other places.  They appear to be created by Core Foundation code.

Found a way for CFStringGetBytes() to return an 8 bit Latin1 string that we can use directly.  No need to check for 8 bit characters.  Again in my simple browsing test this worked every time.

If I need to check for all 8 bit characters in the future, I think modifying and renaming ASCIIFastPath.h to CharacterFastPath.h is the way to go.

[Build Bot]

Comment on attachment 169039 [details]
Updated Patch

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

New failing tests:
platform/mac/editing/pasteboard/text-precomposed.html

[Michael Saboff]

Created attachment 169206 [details]
Patch with Test Failure Fix

Added code to check that all characters in the CFString were used in the conversion to Latin1.

[Michael Saboff]

Created attachment 169251 [details]
Fixed WTF/ChangeLog

No other change from prior patch.

[Geoffrey Garen]

Comment on attachment 169251 [details]
Fixed WTF/ChangeLog

r=me

[WebKit Review Bot]

Comment on attachment 169251 [details]
Fixed WTF/ChangeLog

Clearing flags on attachment: 169251

Committed r131656: <http://trac.webkit.org/changeset/131656>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.