Bug 133678 – TextCodecICU::encode turns the whole string as yen signs if there is any backslash in it

• Component: Text
• Status: RESOLVED FIXED

[youenn fablet]

TextCodecICU::encode is replacing '\' by '¥'.
But it is also doing so for the characters afer '\'.
The string "\cba" will be encoded as '¥¥¥¥'.

[youenn fablet]

Created attachment 232787 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 232787 [details]
Patch

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".

[youenn fablet]

Created attachment 233137 [details]
Updated test and title

[youenn fablet]

(In reply to comment #2)
> (From update of attachment 232787 [details])
> 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".

Thanks for the review.
The uploaded patch is updated according your suggestions.

[Build Bot]

Comment on attachment 233137 [details]
Updated test and title

Attachment 233137 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5250254710505472

New failing tests:
media/W3C/video/networkState/networkState_during_loadstart.html

[Build Bot]

Created attachment 233138 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[WebKit Commit Bot]

Comment on attachment 233137 [details]
Updated test and title

Clearing flags on attachment: 233137

Committed r170573: <http://trac.webkit.org/changeset/170573>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.