[webkit-reviews] review granted: [Bug 46958] Add additional WKString API : [Attachment 69511] Updated, now includes tests!

[bugzilla-daemon at webkit.org]

Kenneth Rohde Christiansen <kenneth at webkit.org> has granted Sam Weinig
<sam at webkit.org>'s request for review:
Bug 46958: Add additional WKString API
https://bugs.webkit.org/show_bug.cgi?id=46958

Attachment 69511: Updated, now includes tests!
https://bugs.webkit.org/attachment.cgi?id=69511&action=review

------- Additional Comments from Kenneth Rohde Christiansen
<kenneth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=69511&action=review

Looks good. r=me

> WebKit2/Shared/WebString.h:62
> +	   WTF::Unicode::ConversionResult result =
WTF::Unicode::convertUTF16ToUTF8(&d, d + m_string.length(), &p, p + bufferSize
- 1, true);

What does the true mean? maybe add a comment like /* ... */ in front of it?

> WebKitTools/TestWebKitAPI/Tests/WebKit2/WKString.cpp:34
> +    WKStringRef string = WKStringCreateWithUTF8CString("hello");

Shouldn't you try constructing it using a "string" containing UTF8 specific
chars?