[webkit-reviews] review granted: [Bug 101678] WTFString::utf8() should have a mode of conversion to use replacement character : [Attachment 173204] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Kenichi Ishibashi
<bashi at chromium.org>'s request for review:
Bug 101678: WTFString::utf8() should have a mode of conversion to use
replacement character
https://bugs.webkit.org/show_bug.cgi?id=101678

Attachment 173204: Patch
https://bugs.webkit.org/attachment.cgi?id=173204&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
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:75
3
>> +					      
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.