code review 4999042: exp/template/html: elide comments in template source.

exp/template/html: elide comments in template source.

When templates are stored in external files, developers often embed
comments to explain&|disable code.

  <!-- Oblique reference to project code name here -->
  {{if .C}}...{{else}}<!-- commented out default -->{{end}}

This unnecessarily increases the size of shipped HTML and can leak
information.

This change elides all comments of the following types:
1. <!-- ... --> comments found in source.
2. /*...*/ and // comments found in <script> elements.
3. /*...*/ and // comments found in <style> elements.

It does not elide /*...*/ or // comments found in HTML attributes:
4. <button onclick="/*...*/">
5. <div style="/*...*/">

I can find no examples of comments in attributes in Closure Templates
code and doing so would require keeping track of character positions
post decode in

  <button onclick="/&#42;...*/">

To prevent token joining, /*comments*/ are JS and CSS comments are
replaced with a whitespace char.
HTML comments are not, but to prevent token joining we could try to
detect cases like
   <<!---->b>
   </<!---->b>
which has a well defined meaning in HTML but will cause a validator
to barf.  This is difficult, and this is a very minor case.
I have punted for now, but if we need to address this case, the best
way would be to normalize '<' in stateText to '&lt;' consistently.

The whitespace to replace a JS /*comment*/ with depends on whether
there is an embedded line terminator since
    break/*
    */foo
    ...
is equivalent to
    break;
    foo
    ...
while
    break/**/foo
    ...
is equivalent to
    break foo;
    ...

Comment eliding can interfere with IE conditional comments.
http://en.wikipedia.org/wiki/Conditional_comment

<!--[if IE 6]>
<p>You are using Internet Explorer 6.</p>
<![endif]-->

/*@cc_on
  document.write("You are using IE4 or higher");
@*/

I have not encountered these in production template code, and
the typed content change in CL 4962067 provides an escape-hatch
if conditional comments are needed.

[MikeSamuel, 2011-09-19 02:19:04]

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

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

[nigeltao, 2011-09-19 05:49:07]

LGTM.

Can we also get some tests for:
`<button onclick="/*...*/">`
"<script>var a/*b*//c\nd</script>"
"<script>var a/*b*///c\nd</script>"

http://codereview.appspot.com/4999042/diff/30004/src/pkg/exp/template/html/co...
File src/pkg/exp/template/html/context.go (right):

http://codereview.appspot.com/4999042/diff/30004/src/pkg/exp/template/html/co...
src/pkg/exp/template/html/context.go:93: stateComment
It might be worth renaming this to stateHTMLCmt. It looks a little weird that
isCommentState(c) returns true for states other than stateComment.

http://codereview.appspot.com/4999042/diff/30004/src/pkg/exp/template/html/co...
src/pkg/exp/template/html/context.go:170: func isCommentState(s state) bool {
You could drop the "State" if this was a method:
func (s state) isComment() bool {

http://codereview.appspot.com/4999042/diff/30004/src/pkg/exp/template/html/es...
File src/pkg/exp/template/html/escape.go (right):

http://codereview.appspot.com/4999042/diff/30004/src/pkg/exp/template/html/es...
src/pkg/exp/template/html/escape.go:559: // start.
In general code, you're not constrained to 80 chars. That limit is only for doc
comments.

http://codereview.appspot.com/4999042/diff/30004/src/pkg/exp/template/html/es...
File src/pkg/exp/template/html/escape_test.go (right):

http://codereview.appspot.com/4999042/diff/30004/src/pkg/exp/template/html/es...
src/pkg/exp/template/html/escape_test.go:369: "Split HTML comment",
I guess this works, but my naive instinct would be that it would be simpler to
say that it's an error to end a text node in a comment state.

http://codereview.appspot.com/4999042/diff/30004/src/pkg/exp/template/html/es...
src/pkg/exp/template/html/escape_test.go:384: // Newline separates break from
call.  If newline
Single space after full stop. Same below.

[MikeSamuel, 2011-09-19 20:56:37]

I addressed most of these issues, but I'm going to put this CL on hold.  Your
suggested testcases exposed a few problems.

I have a series of 4 smaller CLs starting with http://mondrian/5074041 which I
think will lead to a better result.

If it's OK by you,
(1) CL 5074041 simplifies escapeText and the transition.go in a non-semantics
changing way.
(2) A second CL will rename stateComment as requested and introduce isComment.
(3) A third CL will change escapeAction to escape comments so that every action
inside a comment produces the empty string.  This will allow commenting out of
actions in templates and will introduce most of the testcases in this CL.
(4) A final CL will change escapeText to do comment elision and elide comments
in the testcases introduced in (3).

[MikeSamuel, 2011-09-22 02:23:23]

Most of this CL is now gone.  Only the changes to escapeText and tests remain.

[nigeltao, 2011-09-22 03:39:17]

LGTM.

[MikeSamuel, 2011-09-22 04:38:46]

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

exp/template/html: elide comments in template source.

When templates are stored in external files, developers often embed
comments to explain&|disable code.

  <!-- Oblique reference to project code name here -->
  {{if .C}}...{{else}}<!-- commented out default -->{{end}}

This unnecessarily increases the size of shipped HTML and can leak
information.

This change elides all comments of the following types:
1. <!-- ... --> comments found in source.
2. /*...*/ and // comments found in <script> elements.
3. /*...*/ and // comments found in <style> elements.

It does not elide /*...*/ or // comments found in HTML attributes:
4. <button onclick="/*...*/">
5. <div style="/*...*/">

I can find no examples of comments in attributes in Closure Templates
code and doing so would require keeping track of character positions
post decode in

  <button onclick="/&#42;...*/">

To prevent token joining, /*comments*/ are JS and CSS comments are
replaced with a whitespace char.
HTML comments are not, but to prevent token joining we could try to
detect cases like
   <<!---->b>
   </<!---->b>
which has a well defined meaning in HTML but will cause a validator
to barf.  This is difficult, and this is a very minor case.
I have punted for now, but if we need to address this case, the best
way would be to normalize '<' in stateText to '&lt;' consistently.

The whitespace to replace a JS /*comment*/ with depends on whether
there is an embedded line terminator since
    break/*
    */foo
    ...
is equivalent to
    break;
    foo
    ...
while
    break/**/foo
    ...
is equivalent to
    break foo;
    ...

Comment eliding can interfere with IE conditional comments.
http://en.wikipedia.org/wiki/Conditional_comment

<!--[if IE 6]>
<p>You are using Internet Explorer 6.</p>
<![endif]-->

/*@cc_on
  document.write("You are using IE4 or higher");
@*/

I have not encountered these in production template code, and
the typed content change in CL 4962067 provides an escape-hatch
if conditional comments are needed.

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