Bug 101678 – WTFString::utf8() should have a mode of conversion to use replacement character

• Status: RESOLVED
• Resolution: FIXED
• Component: Text

[Kenichi Ishibashi]

WebIDL defines a way to convert a DOMString to a sequence of Unicode characters:
http://dev.w3.org/2006/webapi/WebIDL/#dfn-obtain-unicode

This algorithm is needed to fix https://bugs.webkit.org/show_bug.cgi?id=101569.

In https://bugs.webkit.org/show_bug.cgi?id=101569#c4, Alexey suggested me implementing it under WTF. I think changing String::utf8() to take mode argument is a reasonable approach.

[Kenichi Ishibashi]

Created attachment 173184 [details]
Patch

[Kenichi Ishibashi]

Hi Alexey,

Could you take a look (or could you suggest a reviewer)?

[WebKit Review Bot]

Attachment 173184 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.cpp:753:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexey Proskuryakov]

Comment on attachment 173184 [details]
Patch

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

I haven't looked in detail yet. This approach seems much nicer than adding code to WebSocket.

> Source/WTF/wtf/text/WTFString.cpp:795
> +                // Conversion fails when there is an unpaired surrogate.

How did you confirm that this is the only possible reason for failure?

> Source/WTF/wtf/text/WTFString.h:218
> +        ReplacementConversion,

This name is not descriptive.  When exactly is the replacement character used? Is this actually StrictConversionReplacingUnpairedSurrogatesWithFFFD?

[Build Bot]

Comment on attachment 173184 [details]
Patch

Attachment 173184 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14775372

[Kenichi Ishibashi]

Comment on attachment 173184 [details]
Patch

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

>> Source/WTF/wtf/text/WTFString.cpp:795
>> +                // Conversion fails when there is an unpaired surrogate.
> 
> How did you confirm that this is the only possible reason for failure?

I read the comments and implementation of convertUTF16ToUTF8(). All failures except for "targetExhausted" caused by an unpaired surrogate.

>> Source/WTF/wtf/text/WTFString.h:218
>> +        ReplacementConversion,
> 
> This name is not descriptive.  When exactly is the replacement character used? Is this actually StrictConversionReplacingUnpairedSurrogatesWithFFFD?

Will change.

[Kenichi Ishibashi]

(In reply to comment #5)
> (From update of attachment 173184 [details])
> Attachment 173184 [details] did not pass win-ews (win):
> Output: http://queues.webkit.org/results/14775372

15>WebKit.exp : error LNK2001: unresolved external symbol "public: class WTF::CString __thiscall WTF::String::utf8(bool)const " (?utf8@String@WTF@@QBE?AVCString@2@_N@Z)

It seems that I need to update Source/WebKit2/win/{WebKit2,WebKit2CFLite}.def

How can I generate the entry for "CString utf8(ConversionMode = LenientConversion) const;" ?

[Kenichi Ishibashi]

Created attachment 173204 [details]
Patch

[WebKit Review Bot]

Attachment 173204 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.cpp:753:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexey Proskuryakov]

Comment on attachment 173204 [details]
Patch

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

I would have structured this code differently, not adding a branch for StrictConversionReplacingUnpairedSurrogatesWithFFFD unless a failure is returned from convertUTF16ToUTF8(). Seems acceptable to leave as is.

> Source/WTF/wtf/text/WTFString.cpp:795
> +                // Conversion fails when there is an unpaired surrogate.

Can you add an ASSERT to that effect?

> Source/WTF/wtf/text/WTFString.cpp:799
> +                    ASSERT((buffer + 3) <= (buffer + bufferVector.size()));

You can use bufferEnd at the right side.

> Source/WTF/wtf/text/WTFString.cpp:809
> +            // Only produced from strict conversion.

Can you add an ASSERT to that effect?

>> Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.cpp:753
>> +                                           credential.password().utf8(String::StrictConversion).data(),
> 
> Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]

Style bot is correct, we do not try to align source code like this. Continuation line should be just indented additional 4 spaces.

[Kenichi Ishibashi]

Created attachment 173498 [details]
Patch for landing

[Kenichi Ishibashi]

Comment on attachment 173204 [details]
Patch

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

Thank you for review!

>> Source/WTF/wtf/text/WTFString.cpp:795
>> +                // Conversion fails when there is an unpaired surrogate.
> 
> Can you add an ASSERT to that effect?

Done.

>> Source/WTF/wtf/text/WTFString.cpp:799
>> +                    ASSERT((buffer + 3) <= (buffer + bufferVector.size()));
> 
> You can use bufferEnd at the right side.

Done.

>> Source/WTF/wtf/text/WTFString.cpp:809
>> +            // Only produced from strict conversion.
> 
> Can you add an ASSERT to that effect?

Done.

>>> Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.cpp:753
>>> +                                           credential.password().utf8(String::StrictConversion).data(),
>> 
>> Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> 
> Style bot is correct, we do not try to align source code like this. Continuation line should be just indented additional 4 spaces.

Done.

[WebKit Review Bot]

Comment on attachment 173498 [details]
Patch for landing

Rejecting attachment 173498 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/Source/JavaScriptCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/14810071

[Kenichi Ishibashi]

Created attachment 173499 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 173499 [details]
Patch for landing

Clearing flags on attachment: 173499

Committed r134173: <http://trac.webkit.org/changeset/134173>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.