code review 3445041: html: unescape numeric entities, and complete the named...

html: unescape numeric entities, and complete the named entities table, including two-character entities.

Fixes issue 1233.

[rmmh, 2010-12-03 07:16:23]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),

I'd like you to review this change.

[rsc1, 2010-12-03 16:41:27]

R=nigeltao

[nigeltao, 2010-12-05 10:34:49]

If you haven't already done so, you will also need to complete a Contributor
License Agreement, as per http://golang.org/doc/contribute.html#copyright

http://codereview.appspot.com/3445041/diff/2001/src/pkg/html/entity.go
File src/pkg/html/entity.go (right):

http://codereview.appspot.com/3445041/diff/2001/src/pkg/html/entity.go#newcod...
src/pkg/html/entity.go:2158: // HTML entities that are two unicode codepoints
Comments need to end in a full stop.

http://codereview.appspot.com/3445041/diff/2001/src/pkg/html/entity.go#newcod...
src/pkg/html/entity.go:2159: var entityDouble = map[string][2]int{
Maybe just call the variable entity2.

http://codereview.appspot.com/3445041/diff/2001/src/pkg/html/escape.go
File src/pkg/html/escape.go (right):

http://codereview.appspot.com/3445041/diff/2001/src/pkg/html/escape.go#newcode14
src/pkg/html/escape.go:14: // legacy entity number replacements (from
iso-8859-1)
It's from Windows-1252, not ISO-8859-1, but comment should say that the source
for this table is actually
http://www.whatwg.org/specs/web-apps/current-work/multipage/tokenization.html...

Also, comments should be complete English sentences, and hence start with a
capital letter and end with a full stop. This also applies to many new comments
below.

http://codereview.appspot.com/3445041/diff/2001/src/pkg/html/escape.go#newcode15
src/pkg/html/escape.go:15: var reptable = map[int]int{
I'd call it replacementTable.

http://codereview.appspot.com/3445041/diff/2001/src/pkg/html/escape.go#newcode16
src/pkg/html/escape.go:16: 0x00: '\U0000FFFD', 0x0D: '\U0000000D',
One entry per line, not two. Also, I'd drop 0x0D (and add a comment noting
that). I know it's in the spec, but it's a no-op here, and (modulo the 0x00 ->
'\U0000FFFD' case) could let us implement the replacement table by an array
instead of a map.

http://codereview.appspot.com/3445041/diff/2001/src/pkg/html/escape.go#newcode37
src/pkg/html/escape.go:37: // Precondition: src[0] == '&' && dst <= src.
This is not your fault, but can you fix the comment bug by replacing "src[0]"
with "b[src]"?

http://codereview.appspot.com/3445041/diff/2001/src/pkg/html/escape.go#newcode45
src/pkg/html/escape.go:45: copy(b[dst:dst+1], b[src:src+1])
b[dst] = b[src]

http://codereview.appspot.com/3445041/diff/2001/src/pkg/html/escape.go#newcode52
src/pkg/html/escape.go:52: case '\t', '\n', '\f', ' ', '<', '&':
I know that the spec calls out these cases out differently, but until the
tokenizer does something with parse errors, I'd just let these characters be
handled by the default case, and this switch just becomes
if s[1] == '#' { ... } else { ... }

http://codereview.appspot.com/3445041/diff/2001/src/pkg/html/escape.go#newcode90
src/pkg/html/escape.go:90: fmt.Sscanf(string(s[3:i]), "%x", &x)
Allocating a temporary string just so that you can call fmt.Sscanf is
inefficient. Just accumulate x in the "for i < len(s)" loop above. For example,
at the appropriate point:
x = 10*x + c - '0'

http://codereview.appspot.com/3445041/diff/2001/src/pkg/html/escape.go#newcode96
src/pkg/html/escape.go:96: x1 := reptable[x]
You can combine this line and the next one:
if x1 := reptable[x]; x1 != 0 {

Similarly at lines 130 and 135.

http://codereview.appspot.com/3445041/diff/2001/src/pkg/html/escape.go#newcod...
src/pkg/html/escape.go:113: //  consumed characters matching one of the named
references
Whitespace after "//".

http://codereview.appspot.com/3445041/diff/2001/src/pkg/html/escape.go#newcod...
src/pkg/html/escape.go:114: // (case-sensitive)
I'd drop the "(case-sensitive)" remark.

http://codereview.appspot.com/3445041/diff/2001/src/pkg/html/escape.go#newcod...
src/pkg/html/escape.go:117: // unespace("&notit;") should be "¬it;"
Typo in unescape.

Also, drop the NOTE, just make it: "TODO(nigeltao): unescape("&notit;") should
be "¬it;""

http://codereview.appspot.com/3445041/diff/2001/src/pkg/html/escape.go#newcod...
src/pkg/html/escape.go:135: x := entityDouble[string(s[1:i])]
Make string(s[1:i]) a variable just above line 130, so you don't allocate the
string twice.

http://codereview.appspot.com/3445041/diff/2001/src/pkg/html/token_test.go
File src/pkg/html/token_test.go (right):

http://codereview.appspot.com/3445041/diff/2001/src/pkg/html/token_test.go#ne...
src/pkg/html/token_test.go:111: // The HTML text node to parse.
// The HTML text.

http://codereview.appspot.com/3445041/diff/2001/src/pkg/html/token_test.go#ne...
src/pkg/html/token_test.go:113: // The string representations of the expected
unescaped text.
// The unescaped text.

http://codereview.appspot.com/3445041/diff/2001/src/pkg/html/token_test.go#ne...
src/pkg/html/token_test.go:114: text string
I'd call the field "unescaped" instead of "text".

http://codereview.appspot.com/3445041/diff/2001/src/pkg/html/token_test.go#ne...
src/pkg/html/token_test.go:124: // Simple example: a few named entities
These comments should end with a full stop.

Also, the comments for the first two cases have a different style of comment
than the others. I'd change the first two to be consistent.

http://codereview.appspot.com/3445041/diff/2001/src/pkg/html/token_test.go#ne...
src/pkg/html/token_test.go:130: // Correctly handle hitting the end of the
string
Drop the "Correctly".

http://codereview.appspot.com/3445041/diff/2001/src/pkg/html/token_test.go#ne...
src/pkg/html/token_test.go:132: "stringend",
I'd use camelCase, with interior capitals at what would normally be word
boundaries.

http://codereview.appspot.com/3445041/diff/2001/src/pkg/html/token_test.go#ne...
src/pkg/html/token_test.go:139: "text &AElig; blah",
AElig; isn't two codepoints, right? I think that you want something like bne;
instead.

http://codereview.appspot.com/3445041/diff/2001/src/pkg/html/token_test.go#ne...
src/pkg/html/token_test.go:172: t.Errorf("UnescapeString(%q) == %q != %q",
tt.html, unescaped, tt.text)
t.Errorf("TestUnescape %s: want %q got %q", tt.desc, tt.text, unescaped)

[nigeltao, 2010-12-05 10:43:42]

Also, can you "hg change 3445041" and edit the changelist description so that
"Fixes issue 1233." is on its own separate line. This will let the codereview
close the bug automatically when it gets submitted. Also, the CL description
should start with a one sentence summary prefixed by the package name. In this
case, I think a single sentence is enough. I'd rewrite the CL description as:

"html: unescape numeric entities, and complete the named entities table,
including two-character entities.

Fixes issue 1233."

[rmmh, 2010-12-05 12:20:18]

Hello nigeltao (cc: golang-dev@googlegroups.com),

Please take another look.

[rmmh, 2010-12-05 12:27:26]

On 2010/12/05 10:34:49, nigeltao wrote:
> If you haven't already done so, you will also need to complete a Contributor
> License Agreement, as per http://golang.org/doc/contribute.html#copyright

I completed a CLA and fixed the issues you pointed out.

[nigeltao, 2010-12-06 02:54:36]

This is looking pretty close.

http://codereview.appspot.com/3445041/diff/12001/src/pkg/html/entity.go
File src/pkg/html/entity.go (right):

http://codereview.appspot.com/3445041/diff/12001/src/pkg/html/entity.go#newco...
src/pkg/html/entity.go:2256: // We verify that the length of UTF-8 encoding of
each value is <= 1 + len(key).
Can you do a similar test for entity2?

http://codereview.appspot.com/3445041/diff/12001/src/pkg/html/escape.go
File src/pkg/html/escape.go (right):

http://codereview.appspot.com/3445041/diff/12001/src/pkg/html/escape.go#newco...
src/pkg/html/escape.go:17: '\U000020AC', // First entry is what 0x80 should be
replaced with.
Since all these entries are under \U00010000, I'd use the 4-digit \u20AC
notation instead of the 8-digit \U000020AC.

This doesn't apply to entity.go, obviously.

http://codereview.appspot.com/3445041/diff/12001/src/pkg/html/escape.go#newco...
src/pkg/html/escape.go:49: // Skipped replacements: 0x00->'\U0000FFFD' and
0x0D->'\U0000000D'.
Add that 0x00 is handled programatically and 0x0D is a no-op.

http://codereview.appspot.com/3445041/diff/12001/src/pkg/html/escape.go#newco...
src/pkg/html/escape.go:79: var x int = 0
I'd just say "x := 0", and remove the following empty line.

http://codereview.appspot.com/3445041/diff/12001/src/pkg/html/escape.go#newco...
src/pkg/html/escape.go:113: }
I'd like an
} else if x == 0 || (0xD800 <= x && x <= 0xDFFF) || x > 0x10FFFF {
  x = '\uFFFD'
}

Note that this also checks x==0.

http://codereview.appspot.com/3445041/diff/12001/src/pkg/html/escape.go#newco...
src/pkg/html/escape.go:119: } else {
Since the line above is a return, this "} else {" could just be "}", out-dent
lines 120-146, and delete the "}" at line 147.

http://codereview.appspot.com/3445041/diff/12001/src/pkg/html/escape.go#newco...
src/pkg/html/escape.go:123: // TODO(nigeltao): unespace("&notit;") should be
"¬it;"
Typo: s/unespace/unescape/.

http://codereview.appspot.com/3445041/diff/12001/src/pkg/html/escape.go#newco...
src/pkg/html/escape.go:140: } else {
I think this could be
} else if x := entity2[entityName]; x[0] != 0 {

http://codereview.appspot.com/3445041/diff/12001/src/pkg/html/token_test.go
File src/pkg/html/token_test.go (right):

http://codereview.appspot.com/3445041/diff/12001/src/pkg/html/token_test.go#n...
src/pkg/html/token_test.go:140: "text ⋛︀ blah",
The non-ASCII characters here aren't in my default font. It would be better to
write it as "text \u22DB\uFE00 blah", and probably the same for the other
non-ASCII characters in these tests (even though UTF-8 is valid Go).

[rmmh, 2010-12-06 05:47:26]

Hello nigeltao (cc: golang-dev@googlegroups.com),

Please take another look.

[nigeltao, 2010-12-06 06:32:03]

Just a couple of nits, and I think this LGTM.

I'll hold off on submitting this, though, until I submit the clashing 3355041 so
I can have a clean checkout.

I'll handle the merge. I can also handle the nits, if you'd prefer.

http://codereview.appspot.com/3445041/diff/20001/src/pkg/html/entity.go
File src/pkg/html/entity.go (right):

http://codereview.appspot.com/3445041/diff/20001/src/pkg/html/entity.go#newco...
src/pkg/html/entity.go:2160: // TODO(nigeltao): Handle replacements that are
wider than their names.
Ugh. :-(

http://codereview.appspot.com/3445041/diff/20001/src/pkg/html/escape.go
File src/pkg/html/escape.go (right):

http://codereview.appspot.com/3445041/diff/20001/src/pkg/html/escape.go#newco...
src/pkg/html/escape.go:110: if 0x80 <= x && x <= 0x9f {
s/f/F/ to be consistent with the rest of the file.

http://codereview.appspot.com/3445041/diff/20001/src/pkg/html/escape.go#newco...
src/pkg/html/escape.go:118: // Ranges that should be parse errors are left
unhandled.
I think we can drop this line. I suspect that the Go html package won't ever
call out parse errors.

http://codereview.appspot.com/3445041/diff/20001/src/pkg/html/escape.go#newco...
src/pkg/html/escape.go:121: 
Remove this empty line.

http://codereview.appspot.com/3445041/diff/20001/src/pkg/html/token_test.go
File src/pkg/html/token_test.go (right):

http://codereview.appspot.com/3445041/diff/20001/src/pkg/html/token_test.go#n...
src/pkg/html/token_test.go:140: "text \u22db\ufe00 blah", // Glyph is ⋛︀.
Drop the comment.

http://codereview.appspot.com/3445041/diff/20001/src/pkg/html/token_test.go#n...
src/pkg/html/token_test.go:142: // Handle decimal numeric entities
Missing full stop.
(Same at line 148.)

http://codereview.appspot.com/3445041/diff/20001/src/pkg/html/token_test.go#n...
src/pkg/html/token_test.go:157: "&# &#x &copy = &#169f = &#xa9",
Can I also test something like "&#123;45" with digits after the semi-colon?

[rmmh, 2010-12-06 07:07:03]

Hello nigeltao (cc: golang-dev@googlegroups.com),

Please take another look.

[rmmh, 2010-12-06 07:08:46]

Should be everything.

http://codereview.appspot.com/3445041/diff/20001/src/pkg/html/entity.go
File src/pkg/html/entity.go (right):

http://codereview.appspot.com/3445041/diff/20001/src/pkg/html/entity.go#newco...
src/pkg/html/entity.go:2160: // TODO(nigeltao): Handle replacements that are
wider than their names.
On the bright side, I doubt people will use 	≪⃒ and	≫⃒ much in production. A
simple fix would be using nLtv; and nGtv;-- ≪̸ and	≫̸ seem to be adequate
substitutes.

[nigeltao, 2010-12-07 01:13:57]

*** Submitted as http://code.google.com/p/go/source/detail?r=a3230f45294e ***

html: unescape numeric entities, and complete the named entities table,
including two-character entities.

Fixes issue 1233.

R=nigeltao
CC=golang-dev
http://codereview.appspot.com/3445041

Committer: Nigel Tao <nigeltao@golang.org>