Summary: | TextCodecICU::encode turns the whole string as yen signs if there is any backslash in it | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | youenn fablet <youennf> | ||||||||
Component: | Text | Assignee: | youenn fablet <youennf> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | ap, buildbot, commit-queue, rniwa | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 109199 | ||||||||||
Attachments: |
|
Description
youenn fablet
2014-06-10 08:20:10 PDT
Created attachment 232787 [details]
Patch
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". Created attachment 233137 [details]
Updated test and title
(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. 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 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
Comment on attachment 233137 [details] Updated test and title Clearing flags on attachment: 233137 Committed r170573: <http://trac.webkit.org/changeset/170573> All reviewed patches have been landed. Closing bug. |