[webkit-reviews] review granted: [Bug 50708] Parse error during XSLT transformation : [Attachment 75999] Patch to fix the issue

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Jasmin Lapalme
<jasminlapalme at me.com>'s request for review:
Bug 50708: Parse error during XSLT transformation
https://bugs.webkit.org/show_bug.cgi?id=50708

Attachment 75999: Patch to fix the issue
https://bugs.webkit.org/attachment.cgi?id=75999&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=75999&action=review

This looks great, thanks! I've had a number of trivial nitpicks, at least a few
of which need to be fixed before landing.

I'm marking this r+, so a committer can fix these before landing. Or ideally,
you could upload an updated patch, and then we'll land through commit-queue.

> WebCore/ChangeLog:11
> +	   * xml/XSLTProcessorLibxslt.cpp:
> +	   (WebCore::writeToVector):

This auto-generated list is intended to be completed with a brief per-function
description of changes. Many active contributors don't do that, but it's a
useful practice.

> WebCore/xml/XSLTProcessorLibxslt.cpp:50
>  #include <wtf/text/CString.h>
>  #include <wtf/Vector.h>
> +#include <wtf/unicode/UTF8.h>

A pre-existing issue: we sort headers in ASCII order (same as UNIX sort command
or Xcode Sort Selection script). So upper case comes first.

> WebCore/xml/XSLTProcessorLibxslt.cpp:166
> +    if (len == 0)

WebKit style is "if (!len)".

> WebCore/xml/XSLTProcessorLibxslt.cpp:171
> +    UChar* bufferUChar;
> +    String stringBuffer(StringImpl::createUninitialized(len, bufferUChar));
> +    UChar* bufferUCharEnd = bufferUChar + len;

I think that StringBuffer would look better here. String::fromUTF8() code that
you copied this from probably predates StringBuffer.

> WebCore/xml/XSLTProcessorLibxslt.cpp:173
> +    // Try converting into the buffer.

Why try? Can converting really fail?

Simply converting into a buffer doesn't seem worth a comment to me.

> WebCore/xml/XSLTProcessorLibxslt.cpp:176
> +    WTF::Unicode::ConversionResult result =
> +	   WTF::Unicode::convertUTF8ToUTF16(&stringCurrent, buffer + len,
&bufferUChar, bufferUCharEnd);

No need to wrap this line, it's short enough.

> WebCore/xml/XSLTProcessorLibxslt.cpp:177
> +    if (result == WTF::Unicode::conversionOK || result ==
WTF::Unicode::sourceExhausted) {

Common WebKit style it to have early return on error case, not to nest "if"s
for success cases.

> WebCore/xml/XSLTProcessorLibxslt.cpp:183
>      return len;

Is this correct error handling? Per
<http://xmlsoft.org/html/libxml-xmlIO.html#xmlOutputWriteCallback>, -1 should
be returned.

Also, looks like we don't really ever expect any errors here, so
ASSERT_NOT_REACHED() would be good, with or without release build handling.

> LayoutTests/ChangeLog:10
> +	   * fast/xsl/xslt-bad-encoding-expected.txt: Added.
> +	   * fast/xsl/xslt-bad-encoding.xml: Added.
> +	   * fast/xsl/xslt-bad-encoding.xsl: Added.

This is not a good name for the test, as the source files are all properly
encoded. I'd suggest something like "utf8-chunks.xml".

> LayoutTests/fast/xsl/xslt-bad-encoding.xml:30
>
+ééééééééééééééééééééééééééééééééééééééé
éééééééééé

Is this enough to reliably trigger the bug under different versions of libxml2
and other XML back-ends? Maybe make this section longer, just to be safe?

> LayoutTests/fast/xsl/xslt-bad-encoding.xsl:3
> +			       
xmlns:xsl="http://www.w3.org/1999/XSL/Transform">

Our subversion pre-commit hooks will complain about tabs in files. Please
convert to spaces.