[webkit-reviews] review granted: [Bug 133678] TextCodecICU::encode should not change characters other characters than backslash : [Attachment 232787] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted youenn fablet
<youennf at gmail.com>'s request for review:
Bug 133678: TextCodecICU::encode should not change characters other characters
than backslash
https://bugs.webkit.org/show_bug.cgi?id=133678

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

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


> LayoutTests/fast/encoding/backslash-encoding-jp.html:3
> +	   <title>Bug 133678: TextCodecICU::encode should not change characters
other characters than backslash</title>

The title doesn't parse. I'd say "TextCodecICU::encode turns the whole string
into yen signs if there are any backslashes in it". In a bug title, it's better
to describe the symptom, not what the code should do.

Also, it's better to have a <p> element than a <title>, because this way, the
output contains the description of what is being tested, not only a "PASS".

> LayoutTests/fast/encoding/backslash-encoding-jp.html:12
> +	       var toBeEncoded = "\\abcde";
> +	       var expectedEncoded = "%5Cabcde";

This test only checks that characters after the backslash are correct, but the
patch also fixes characters before the backslash, as a separate code change.
I.e. please test "ab\\cde", not "\\abcde".