[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="&#x586f">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