[webkit-reviews] review denied: [Bug 45393] WebKitTestRunner should be portable : [Attachment 69217] proposed patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Balazs Kelemen <kbalazs at webkit.org>'s
request for review:
Bug 45393: WebKitTestRunner should be portable
https://bugs.webkit.org/show_bug.cgi?id=45393

Attachment 69217: proposed patch
https://bugs.webkit.org/attachment.cgi?id=69217&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=69217&action=review

Seems like a reasonable basic approach, but there are some problems.

> WebKitTools/WebKitTestRunner/StringFunctions.h:80
> +    ASSERT(WKStringGetMaximumUTF8CStringSize(stringRef) <=
staticBufferSize);

We can’t assert something just because we hope it’s true. There’s no guarantee
this will be true, so the code is wrong!

> WebKit2/Shared/API/c/WKString.cpp:48
> +WKStringRef WKStringCreateWithUTF8CString(const char* string)
> +{
> +    return toRef(WebString::create(string).leakRef());
> +}

The name of this function does not match the implementation. The name indicates
the string is treated as UTF-8, but the code actually treats the strings as ISO
Latin-1.

> WebKit2/Shared/API/c/WKString.cpp:73
> +bool WKStringIsEqualToUTF8CString(WKStringRef a, const char* b)
> +{
> +    return *toWK(a) == b;
> +}

This function also won’t work right. The code treats the string as ISO Latin-1,
not UTF-8.

> WebKit2/Shared/API/c/WKURL.cpp:40
> +WKURLRef WKURLCreateWithUTF8CString(const char* string)
> +{
> +    return toRef(WebURL::create(string).leakRef());
> +}

Same problem here. This won’t treat the input as a UTF-8 string.

> WebKit2/Shared/API/c/WKString.h:65
> +WK_EXPORT WKStringRef WKStringCreateWithCharacters(const WKChar* characters,
size_t length);
> +
> +WK_EXPORT WKStringRef WKStringCreateWithUTF8CString(const char* string);
> +
> +WK_EXPORT const WKChar* WKStringGetCharactersPtr(WKStringRef string);
> +
> +WK_EXPORT size_t WKStringGetLength(WKStringRef string);
> +
>  WK_EXPORT bool WKStringIsEmpty(WKStringRef string);
>  
> +WK_EXPORT bool WKStringIsEqual(WKStringRef a, WKStringRef b);
> +
> +WK_EXPORT bool WKStringIsEqualToUTF8CString(WKStringRef a, const char* b);
> +
> +WK_EXPORT size_t WKStringGetMaximumUTF8CStringSize(WKStringRef string);
> +
> +WK_EXPORT size_t WKStringGetUTF8CString(WKStringRef string, char* buffer,
size_t bufferSize);

This might be too many operations. Do we really need all of these? For example,
if we have GetCharactersPtr, I am not sure we need GetUTF8CString.