[webkit-reviews] review granted: [Bug 75707] Many css2.1 tests fail on Apple's Windows port under NRWT : [Attachment 203022] Initial work in progress. UTF-8 encoding is not being handled properly.

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Brent Fulgham <bfulgham at webkit.org>'s
request for review:
Bug 75707: Many css2.1 tests fail on Apple's Windows port under NRWT
https://bugs.webkit.org/show_bug.cgi?id=75707

Attachment 203022: Initial work in progress. UTF-8 encoding is not being
handled properly.
https://bugs.webkit.org/attachment.cgi?id=203022&action=review

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


r=me if you fix the storage leak

> Tools/DumpRenderTree/win/DumpRenderTree.cpp:962
> +String createPathIfURLHasLogueDirectory(const CFURLRef& url)

Crazy use of the word “logue” here to mean prologue or epilogue!?

Type here should just be CFURLRef, not const CFURLRef&.

> Tools/DumpRenderTree/win/DumpRenderTree.cpp:967
> +	   return "";

empytString() is the more efficient alternative to "" when returning a String.

> Tools/DumpRenderTree/win/DumpRenderTree.cpp:971
> +    UInt8* buffer = new UInt8[length];

This buffer leaks. One fix is to use an OwnArrayPtr. Another is to use
Vector<UInt8>.

> Tools/DumpRenderTree/win/DumpRenderTree.cpp:973
> +    if (-1 == length)

Normally we don’t do the “constant on the left” thing like this. Also, it’s
impossible for CFURLGetBytes to return length if we are passing it a length we
got from calling it the first time, so this check is guaranteed dead code.

> Tools/DumpRenderTree/win/DumpRenderTree.cpp:978
> +    String loguePath(buffer + pathRange.location, pathRange.length);
> +
> +    loguePath = WebCore::pathByAppendingComponent(loguePath, "resources/");

Seems like we could do this with CFURL and CFString functions rather than
dropping into WebCore internal types to do it.

CFURLCreateCopyAppendingPathComponent and CFURLGetFileSystemRepresentation
could be used to do the job this function does without involving the
CFURLGetByte functions.

But I suppose this is OK as is.