[webkit-reviews] review denied: [Bug 58058] Replace WKStringGetCharactersPtr() with WKStringGetCharacters() : [Attachment 88659] Patch

[bugzilla-daemon at webkit.org]

Adam Roben (:aroben) <aroben at apple.com> has denied Jeff Miller
<jeffm at apple.com>'s request for review:
Bug 58058: Replace WKStringGetCharactersPtr() with WKStringGetCharacters()
https://bugs.webkit.org/show_bug.cgi?id=58058

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

------- Additional Comments from Adam Roben (:aroben) <aroben at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=88659&action=review

I should be encouraging you to write API tests for the new API you're adding.
They're pretty easy to write; just add some code to
Tools/TestWebKitAPI/Tests/WebKit2/WKString.cpp.

> Source/WebKit2/Shared/WebString.h:68
> +	   ASSERT(buffer);
> +	   if (!buffer)
> +	       return 0;

I don't think we're normally this defensive in the WK2 API.

> Source/WebKit2/Shared/WebString.h:72
> +	   size_t lengthInBytes = m_string.length() * sizeof(UChar);
> +	   if (bufferSize > lengthInBytes)
> +	       bufferSize = lengthInBytes;
> +	   memcpy(buffer, m_string.characters(), bufferSize);

This will cause a buffer overrun if bufferSize is less than lengthInBytes.

> Source/WebKit2/Shared/API/c/WKString.cpp:55
> +size_t WKStringGetCharacters(WKStringRef stringRef, WKChar* buffer, size_t
bufferSize)

It's surprising that this API takes a number bytes to copy rather than a number
of characters.