code review 4186055: html: tokenize HTML comments.

html: tokenize HTML comments.

I'm not sure if it's 100% correct wrt the HTML5 specification,
but the test suite has plenty of HTML comment test cases, and
we'll shake out any tokenization bugs as the parser improves its
coverage.

[nigeltao, 2011-02-16 10:59:59]

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

I'd like you to review this change to
https://go.googlecode.com/hg/

[gri, 2011-02-16 18:56:40]

Some minor comments.

(It does parse the spec though ;-)

-gri

http://codereview.appspot.com/4186055/diff/4001/src/pkg/html/token.go
File src/pkg/html/token.go (right):

http://codereview.appspot.com/4186055/diff/4001/src/pkg/html/token.go#newcode106
src/pkg/html/token.go:106: // ReturnComments is whether or not Next should
return comment tokens.
// If ReturnComments is set, Next returns comment tokens;
// otherwise it skips over comments (default).

http://codereview.appspot.com/4186055/diff/4001/src/pkg/html/token.go#newcode193
src/pkg/html/token.go:193: if err != nil {
if err != nil || c != '-' {

and then you can get rid of the 2nd if in this loop

http://codereview.appspot.com/4186055/diff/4001/src/pkg/html/token.go#newcode200
src/pkg/html/token.go:200: // <!--> is a valid comment.
I remember vaguely that a comment that is never closed also is interpreted as a
TextToken (ie, in the worst case one may have of to read all text and then go
back), but I don't know what HTML5 says.

http://codereview.appspot.com/4186055/diff/4001/src/pkg/html/token.go#newcode202
src/pkg/html/token.go:202: for {
maybe:

for dashCount := 2; ; {

(leave it up to you)

http://codereview.appspot.com/4186055/diff/4001/src/pkg/html/token.go#newcode350
src/pkg/html/token.go:350: // Text returns either a TextToken or CommentToken's
text after unescaping.
Text returns the unescaped text of a TextToken or a CommentToken.

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

http://codereview.appspot.com/4186055/diff/4001/src/pkg/html/token_test.go#ne...
src/pkg/html/token_test.go:56: "abc<b><!-- skipme --></b>def",
you should also have tests for some special cases, at least:
<!-->
<!--->
<!--x>-->
<!--x->-->
and some that are not comments
<!>
<!->
<!---<>
<!--

[nigeltao, 2011-02-16 22:18:46]

PTAL.

There's been a small behavior change. The text "<!-xEOF" now tokenizes as one
text token "&lt;!-xEOF" instead of two text tokens "&lt;!-" followed by "xEOF".

http://codereview.appspot.com/4186055/diff/4001/src/pkg/html/token.go
File src/pkg/html/token.go (right):

http://codereview.appspot.com/4186055/diff/4001/src/pkg/html/token.go#newcode106
src/pkg/html/token.go:106: // ReturnComments is whether or not Next should
return comment tokens.
On 2011/02/16 18:56:41, gri wrote:
> // If ReturnComments is set, Next returns comment tokens;
> // otherwise it skips over comments (default).

Done.

http://codereview.appspot.com/4186055/diff/4001/src/pkg/html/token.go#newcode193
src/pkg/html/token.go:193: if err != nil {
On 2011/02/16 18:56:41, gri wrote:
> if err != nil || c != '-' {
> 
> and then you can get rid of the 2nd if in this loop

On second thoughts, if I want "<!-xEOF" to tokenize as a single text token then
I need separate if cases.

http://codereview.appspot.com/4186055/diff/4001/src/pkg/html/token.go#newcode200
src/pkg/html/token.go:200: // <!--> is a valid comment.
On 2011/02/16 18:56:41, gri wrote:
> I remember vaguely that a comment that is never closed also is interpreted as
a
> TextToken (ie, in the worst case one may have of to read all text and then go
> back), but I don't know what HTML5 says.

I also think HTML5 says that it's a TextToken.

http://codereview.appspot.com/4186055/diff/4001/src/pkg/html/token.go#newcode202
src/pkg/html/token.go:202: for {
On 2011/02/16 18:56:41, gri wrote:
> maybe:
> 
> for dashCount := 2; ; {
> 
> (leave it up to you)

Done.

http://codereview.appspot.com/4186055/diff/4001/src/pkg/html/token.go#newcode350
src/pkg/html/token.go:350: // Text returns either a TextToken or CommentToken's
text after unescaping.
On 2011/02/16 18:56:41, gri wrote:
> Text returns the unescaped text of a TextToken or a CommentToken.

Done.

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

http://codereview.appspot.com/4186055/diff/4001/src/pkg/html/token_test.go#ne...
src/pkg/html/token_test.go:56: "abc<b><!-- skipme --></b>def",
On 2011/02/16 18:56:41, gri wrote:
> you should also have tests for some special cases, at least:
> <!-->
> <!--->
> <!--x>-->
> <!--x->-->
> and some that are not comments
> <!>
> <!->
> <!---<>
> <!--

Done.

[gri, 2011-02-16 23:24:20]

LGTM

[nigeltao, 2011-02-16 23:45:42]

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

html: tokenize HTML comments.

I'm not sure if it's 100% correct wrt the HTML5 specification,
but the test suite has plenty of HTML comment test cases, and
we'll shake out any tokenization bugs as the parser improves its
coverage.

R=gri
CC=golang-dev
http://codereview.appspot.com/4186055