[webkit-reviews] review granted: [Bug 71316] decodeEscapeSequences() not correct for some encodings (GBK, Big5, ...). : [Attachment 123597] New patch for review.
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Jan 23 14:07:13 PST 2012
Daniel Bates <dbates at webkit.org> has granted Thomas Sepez
<tsepez at chromium.org>'s request for review:
Bug 71316: decodeEscapeSequences() not correct for some encodings (GBK, Big5,
...).
https://bugs.webkit.org/show_bug.cgi?id=71316
Attachment 123597: New patch for review.
https://bugs.webkit.org/attachment.cgi?id=123597&action=review
------- Additional Comments from Daniel Bates <dbates at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=123597&action=review
This patch looks good to me. Encoding/decoding can be tricky. If anyone sees
any issues with this patch then feel free to raise them. I had some minor nits.
> Source/WebCore/ChangeLog:9
> + results in face of encodings which re-use ASCII values in sequences.
Nit: I think your missing the word "the" in this line.
> Source/WebCore/platform/text/DecodeEscapeSequences.h:81
> + // 0x40 - 0x7f range, after two values in this range, or at a %-sign
that does not introduce a valid
Nit: You mention 0x7F above and 0x7f here. For consistency, I suggest choosing
one hexadecimal digit notation (either uppercase or lowercase) to use
throughout this comment.
> Source/WebCore/platform/text/DecodeEscapeSequences.h:92
> + } else if (string[runEnd] >= 0x40 && string[runEnd] < 0x80 &&
numberOfTrailingCharacters < 2) {
For your consideration, I suggest writing the conjunct "string[runEnd] < 0x80"
as "string[runEnd] <= 0x7F" so as to be consistent with the usage of
greater-than-equal-to in the first conjunct as well as to make the maximum
value in the range (0x7F) more recognizable, especially after reading the above
comment.
> LayoutTests/http/tests/navigation/anchor-frames-gbk-expected.txt:17
> +This is an anchor point named as the unicode equivalent of the gbk sequence
%a9g (test trailing low byte).
Nit: gbk => GBK
(since GBK is an acronym).
Nit: unicode => Unicode
> LayoutTests/http/tests/navigation/anchor-frames-gbk.html:1
> +<html>
This is the only test in this patch that doesn't have a DOCTYPE. Did you intend
for this test to be run under quirks mode? If so, it may be beneficial to add a
comment to the test to explain this. Regardless of the compatibility mode this
test is run under, I don't anticipate a difference in the expected results .
> LayoutTests/http/tests/navigation/resources/frame-with-anchor-gbk.html:38
> +<a name="塯">This is an anchor point named as the unicode equivalent of
the gbk sequence %a9g (test trailing low byte)</a>.
Nit: gbk => GBK
For your consideration, I suggest programmatically removing this node before
calling finishJSTest() so that this text doesn't appear after "TEST COMPLETE"
in the expected result.
More information about the webkit-reviews
mailing list