code review 4968058: exp/template/html: autoescape actions in HTML style att...

exp/template/html: autoescape actions in HTML style attributes

This does not wire up <style> elements as that is pending support
for raw text content in CL http://codereview.appspot.com/4964045/

This CL allows actions to appear in contexts like

selectors:        {{.Tag}}{{.Class}}{{.Id}}
property names:   border-{{.BidiLeadingEdge}}
property values:  color: {{.Color}}
strings:          font-family: "{{font-name}}"
URL strings:      background: "/foo?image={{.ImgQuery}}"
URL literals:     background: url("{{.Image}}")

but disallows actions inside CSS comments and disallows
embedding of JS in CSS entirely.

It is based on the CSS3 lexical grammar with affordances for
common browser extensions including line comments.

[MikeSamuel, 2011-09-01 23:07:42]

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-02 09:11:37]

This is quite a lot of code to digest. I've made mostly superficial comments for
now. I'll circle back next week.

http://codereview.appspot.com/4968058/diff/21001/src/pkg/exp/template/html/cs...
File src/pkg/exp/template/html/css.go (right):

http://codereview.appspot.com/4968058/diff/21001/src/pkg/exp/template/html/cs...
src/pkg/exp/template/html/css.go:14: // endsWithCSSKeyword is true iff b ends
with an ident and that ident
I'd s/is true iff/returns whether/.

http://codereview.appspot.com/4968058/diff/21001/src/pkg/exp/template/html/cs...
src/pkg/exp/template/html/css.go:37: func isCSSNmchar(rune int) bool {
Go has runes and bytes; we don't normally talk of chars.

js.go defines "func isJSIdentPart". The two names should be consistent.

http://codereview.appspot.com/4968058/diff/21001/src/pkg/exp/template/html/cs...
src/pkg/exp/template/html/css.go:41: switch {
return 'a' <= rune && rune <= 'z' ||
  'A' <= rune && rune <= 'Z' ||
  etc

http://codereview.appspot.com/4968058/diff/21001/src/pkg/exp/template/html/cs...
src/pkg/exp/template/html/css.go:47: // nonascii
I'd write this as
// Non-ASCII cases below.

http://codereview.appspot.com/4968058/diff/21001/src/pkg/exp/template/html/cs...
src/pkg/exp/template/html/css.go:56: // decodeCSS decodes CSS3 escapes given a
sequence of stringchars.
What's a stringchar?

http://codereview.appspot.com/4968058/diff/21001/src/pkg/exp/template/html/cs...
src/pkg/exp/template/html/css.go:106: // isHex returns true iff the given
character is a hex digit.
I'd s/true iff/whether/.

http://codereview.appspot.com/4968058/diff/21001/src/pkg/exp/template/html/cs...
src/pkg/exp/template/html/css.go:111: // hexDecode decodes a short hex digit
sequence. "10" -> 16
I'd s/./:/ and add a trailing full stop.

http://codereview.appspot.com/4968058/diff/21001/src/pkg/exp/template/html/cs...
src/pkg/exp/template/html/css.go:232: var s []byte
Any reason why s is a []byte and not a string?

http://codereview.appspot.com/4968058/diff/21001/src/pkg/exp/template/html/cs...
src/pkg/exp/template/html/css.go:247: s, id := decodeCSS(s), a[:0]
I think s/a[:0]/make([]byte, 0, 64)/ would be more idiomatic. Escape analysis
should mean that the two are equivalent wrt stack vs heap allocations.

http://codereview.appspot.com/4968058/diff/21001/src/pkg/exp/template/html/cs...
src/pkg/exp/template/html/css.go:262: case 0, '"', '\'', '(', ')', '/', ';',
'@', '[', '\\', ']',
I think this would read better without the line break. There is no 80 char limit
for code.

http://codereview.appspot.com/4968058/diff/21001/src/pkg/exp/template/html/cs...
src/pkg/exp/template/html/css.go:264: return "ZgotmplZ"
If you're going to keep using "ZgotmplZ", it should be a constant somewhere
(probably escape.go):

// placeholder is blah blah blah.
const placeholder = "ZgotmplZ"

There's probably a better name than "placeholder".

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

http://codereview.appspot.com/4968058/diff/21001/src/pkg/exp/template/html/es...
src/pkg/exp/template/html/escape.go:36: "exp_template_html_nospaceescaper": 
htmlNospaceEscaper,
You might as well keep this list sorted alphabetically.

http://codereview.appspot.com/4968058/diff/21001/src/pkg/exp/template/html/es...
src/pkg/exp/template/html/escape.go:113: case delimNone:
I like to put an explicit "// No-op." comment (on its on line) for no-op cases.
I think it's less likely to be confusing for somebody used to C/C++'s switch
semantics.

http://codereview.appspot.com/4968058/diff/21001/src/pkg/exp/template/html/ht...
File src/pkg/exp/template/html/html.go (right):

http://codereview.appspot.com/4968058/diff/21001/src/pkg/exp/template/html/ht...
src/pkg/exp/template/html/html.go:15: ok := false
It's probably worth extracting a function for lines 15-22.

http://codereview.appspot.com/4968058/diff/21001/src/pkg/exp/template/html/ht...
src/pkg/exp/template/html/html.go:86: b.WriteString("&#x")
fmt.Fprintf(b, "&#x%04x;", r)

Similarly in url.go.

http://codereview.appspot.com/4968058/diff/21001/src/pkg/exp/template/html/ur...
File src/pkg/exp/template/html/url.go (right):

http://codereview.appspot.com/4968058/diff/21001/src/pkg/exp/template/html/ur...
src/pkg/exp/template/html/url.go:81: case '!', '#', '$', '&', '*', '+', ',',
'/', ':', ';', '=',
I think this would read better without the line break. There is no 80 char limit
for code.

http://codereview.appspot.com/4968058/diff/21001/src/pkg/exp/template/html/ur...
src/pkg/exp/template/html/url.go:116: if b.Len() == 0 {
"if written == 0 {".

[MikeSamuel, 2011-09-02 17:07:43]

Incremental diffs at
http://codereview.appspot.com/4968058/diff2/21001:10004/src/pkg/exp/template/...

http://codereview.appspot.com/4968058/diff/21001/src/pkg/exp/template/html/cs...
File src/pkg/exp/template/html/css.go (right):

http://codereview.appspot.com/4968058/diff/21001/src/pkg/exp/template/html/cs...
src/pkg/exp/template/html/css.go:14: // endsWithCSSKeyword is true iff b ends
with an ident and that ident
On 2011/09/02 09:11:37, nigeltao wrote:
> I'd s/is true iff/returns whether/.

Done.

http://codereview.appspot.com/4968058/diff/21001/src/pkg/exp/template/html/cs...
src/pkg/exp/template/html/css.go:37: func isCSSNmchar(rune int) bool {
On 2011/09/02 09:11:37, nigeltao wrote:
> Go has runes and bytes; we don't normally talk of chars.
> 
> js.go defines "func isJSIdentPart". The two names should be consistent.

"nmchar" is the name of a grammar production in the CSS3 spec.  "IdentifierPart"
is the name of a grammar production in the EscmaScript spec.

http://codereview.appspot.com/4968058/diff/21001/src/pkg/exp/template/html/cs...
src/pkg/exp/template/html/css.go:41: switch {
On 2011/09/02 09:11:37, nigeltao wrote:
> return 'a' <= rune && rune <= 'z' ||
>   'A' <= rune && rune <= 'Z' ||
>   etc

Done.

http://codereview.appspot.com/4968058/diff/21001/src/pkg/exp/template/html/cs...
src/pkg/exp/template/html/css.go:47: // nonascii
On 2011/09/02 09:11:37, nigeltao wrote:
> I'd write this as
> // Non-ASCII cases below.

Done.

http://codereview.appspot.com/4968058/diff/21001/src/pkg/exp/template/html/cs...
src/pkg/exp/template/html/css.go:56: // decodeCSS decodes CSS3 escapes given a
sequence of stringchars.
On 2011/09/02 09:11:37, nigeltao wrote:
> What's a stringchar?

http://www.w3.org/TR/css3-syntax/#SUBTOK-stringchar

stringchar ::= urlchar | #x20 | '\' nl
urlchar ::= [#x9#x21#x23-#x26#x27-#x7E] | nonascii | escape

http://codereview.appspot.com/4968058/diff/21001/src/pkg/exp/template/html/cs...
src/pkg/exp/template/html/css.go:106: // isHex returns true iff the given
character is a hex digit.
On 2011/09/02 09:11:37, nigeltao wrote:
> I'd s/true iff/whether/.

Done.

http://codereview.appspot.com/4968058/diff/21001/src/pkg/exp/template/html/cs...
src/pkg/exp/template/html/css.go:111: // hexDecode decodes a short hex digit
sequence. "10" -> 16
On 2011/09/02 09:11:37, nigeltao wrote:
> I'd s/./:/ and add a trailing full stop.

Done.

http://codereview.appspot.com/4968058/diff/21001/src/pkg/exp/template/html/cs...
src/pkg/exp/template/html/css.go:232: var s []byte
On 2011/09/02 09:11:37, nigeltao wrote:
> Any reason why s is a []byte and not a string?

No real reason.  The decodeCSS call at 247 works on a []byte.  Changed to use
the common coerce function.

http://codereview.appspot.com/4968058/diff/21001/src/pkg/exp/template/html/cs...
src/pkg/exp/template/html/css.go:247: s, id := decodeCSS(s), a[:0]
On 2011/09/02 09:11:37, nigeltao wrote:
> I think s/a[:0]/make([]byte, 0, 64)/ would be more idiomatic. Escape analysis
> should mean that the two are equivalent wrt stack vs heap allocations.

Done.

http://codereview.appspot.com/4968058/diff/21001/src/pkg/exp/template/html/cs...
src/pkg/exp/template/html/css.go:262: case 0, '"', '\'', '(', ')', '/', ';',
'@', '[', '\\', ']',
On 2011/09/02 09:11:37, nigeltao wrote:
> I think this would read better without the line break. There is no 80 char
limit
> for code.

Done.

http://codereview.appspot.com/4968058/diff/21001/src/pkg/exp/template/html/cs...
src/pkg/exp/template/html/css.go:264: return "ZgotmplZ"
On 2011/09/02 09:11:37, nigeltao wrote:
> If you're going to keep using "ZgotmplZ", it should be a constant somewhere
> (probably escape.go):
> 
> // placeholder is blah blah blah.
> const placeholder = "ZgotmplZ"
> 
> There's probably a better name than "placeholder".

Called it filterFailsafe

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

http://codereview.appspot.com/4968058/diff/21001/src/pkg/exp/template/html/es...
src/pkg/exp/template/html/escape.go:36: "exp_template_html_nospaceescaper": 
htmlNospaceEscaper,
On 2011/09/02 09:11:37, nigeltao wrote:
> You might as well keep this list sorted alphabetically.

Done.

http://codereview.appspot.com/4968058/diff/21001/src/pkg/exp/template/html/es...
src/pkg/exp/template/html/escape.go:113: case delimNone:
On 2011/09/02 09:11:37, nigeltao wrote:
> I like to put an explicit "// No-op." comment (on its on line) for no-op
cases.
> I think it's less likely to be confusing for somebody used to C/C++'s switch
> semantics.

// No extra-escaping needed for raw text content

http://codereview.appspot.com/4968058/diff/21001/src/pkg/exp/template/html/ht...
File src/pkg/exp/template/html/html.go (right):

http://codereview.appspot.com/4968058/diff/21001/src/pkg/exp/template/html/ht...
src/pkg/exp/template/html/html.go:15: ok := false
On 2011/09/02 09:11:37, nigeltao wrote:
> It's probably worth extracting a function for lines 15-22.

I put the below in escape.go and used it in 8 places.

// coerceEscaperInput takes varargs inputs and coerces them to a single string.
func coerceEscaperInput(args ...interface{}) string {
	if len(args) == 1 {
		if s, ok := args[0].(string); ok {
			return s
		}
	}
	return fmt.Sprint(args...)
}

http://codereview.appspot.com/4968058/diff/21001/src/pkg/exp/template/html/ht...
src/pkg/exp/template/html/html.go:86: b.WriteString("&#x")
On 2011/09/02 09:11:37, nigeltao wrote:
> fmt.Fprintf(b, "&#x%04x;", r)
> 
> Similarly in url.go.

Done.

I saw the below in template/funcs.go

    // TODO(dsymonds): Do this without fmt?
    fmt.Fprintf(w, "\\u%04X", rune)

and thought that format strings were frowned upon for this sort of thing.

Changing urlProcessor to use 
    fmt.Fprintf(&b, "%%%02x", c)
instead of
    b.WriteByte('%')
    b.WriteByte("0123456789abcdef"[c>>4])
    b.WriteByte("0123456789abcdef"[c&0xf])
causes the benchmarks to take treble the time.

html.BenchmarkURLEscaper          200000              8377 ns/op
html.BenchmarkURLNormalizer       500000              6453 ns/op

vs

html.BenchmarkURLEscaper         1000000              2553 ns/op
html.BenchmarkURLNormalizer      1000000              1852 ns/op

http://codereview.appspot.com/4968058/diff/21001/src/pkg/exp/template/html/ur...
File src/pkg/exp/template/html/url.go (right):

http://codereview.appspot.com/4968058/diff/21001/src/pkg/exp/template/html/ur...
src/pkg/exp/template/html/url.go:81: case '!', '#', '$', '&', '*', '+', ',',
'/', ':', ';', '=',
On 2011/09/02 09:11:37, nigeltao wrote:
> I think this would read better without the line break. There is no 80 char
limit
> for code.

Done.

http://codereview.appspot.com/4968058/diff/21001/src/pkg/exp/template/html/ur...
src/pkg/exp/template/html/url.go:116: if b.Len() == 0 {
On 2011/09/02 09:11:37, nigeltao wrote:
> "if written == 0 {".

Done.

[nigeltao, 2011-09-06 01:54:08]

LGTM.

http://codereview.appspot.com/4968058/diff/21001/src/pkg/exp/template/html/cs...
File src/pkg/exp/template/html/css.go (right):

http://codereview.appspot.com/4968058/diff/21001/src/pkg/exp/template/html/cs...
src/pkg/exp/template/html/css.go:56: // decodeCSS decodes CSS3 escapes given a
sequence of stringchars.
On 2011/09/02 17:07:43, MikeSamuel wrote:
> On 2011/09/02 09:11:37, nigeltao wrote:
> > What's a stringchar?
> 
> http://www.w3.org/TR/css3-syntax/#SUBTOK-stringchar

Can you incorporate that link into the doc comment? Future maintainers might
also not know what a stringchar is.

http://codereview.appspot.com/4968058/diff/21001/src/pkg/exp/template/html/ht...
File src/pkg/exp/template/html/html.go (right):

http://codereview.appspot.com/4968058/diff/21001/src/pkg/exp/template/html/ht...
src/pkg/exp/template/html/html.go:86: b.WriteString("&#x")
On 2011/09/02 17:07:43, MikeSamuel wrote:
> On 2011/09/02 09:11:37, nigeltao wrote:
> > fmt.Fprintf(b, "&#x%04x;", r)
> > 
> > Similarly in url.go.
> 
> Done.
> 
> I saw the below in template/funcs.go
> 
>     // TODO(dsymonds): Do this without fmt?
>     fmt.Fprintf(w, "\\u%04X", rune)
> 
> and thought that format strings were frowned upon for this sort of thing.
> 
> Changing urlProcessor to use 
>     fmt.Fprintf(&b, "%%%02x", c)
> instead of
>     b.WriteByte('%')
>     b.WriteByte("0123456789abcdef"[c>>4])
>     b.WriteByte("0123456789abcdef"[c&0xf])
> causes the benchmarks to take treble the time.
> 
> html.BenchmarkURLEscaper          200000              8377 ns/op
> html.BenchmarkURLNormalizer       500000              6453 ns/op
> 
> vs
> 
> html.BenchmarkURLEscaper         1000000              2553 ns/op
> html.BenchmarkURLNormalizer      1000000              1852 ns/op

Since the benchmarks show 3x, I'm happy to go back to what you had before.

http://codereview.appspot.com/4968058/diff/8003/src/pkg/exp/template/html/css.go
File src/pkg/exp/template/html/css.go (right):

http://codereview.appspot.com/4968058/diff/8003/src/pkg/exp/template/html/css...
src/pkg/exp/template/html/css.go:33: return
bytes.HasSuffix(bytes.ToLower(b[len(b)-len(kw):]), kw)
Can you use bytes.Equal instead of bytes.HasSuffix?

Also, it might read better if you take the "b[len(b)-len(kw):]" out of here and
put
b = b[len(b)-len(kw):]
inside the if block above? It might help to start the function with:
i := len(b) - len(kw)

Finally, if kw is going to be constant (and not untrusted input), it might be
more appropriate for kw to be a string instead of a []byte. The final line could
be:
return string(bytes.ToLower(b)) == kw

The compiler might not currently optimize away the []byte to string conversion,
but it should be able to.

http://codereview.appspot.com/4968058/diff/8003/src/pkg/exp/template/html/css...
src/pkg/exp/template/html/css.go:69: copy(b[len(b):len(b)+i], s[:i])
This could be:
b, s = append(b, s[:i]...), s[i:]

http://codereview.appspot.com/4968058/diff/8003/src/pkg/exp/template/html/css...
src/pkg/exp/template/html/css.go:95: copy(b[len(b):len(b)+n], s[1:])
This could be:
b, s = append(b, s[1:1+n]...), s[1+n:]

http://codereview.appspot.com/4968058/diff/8003/src/pkg/exp/template/html/css...
src/pkg/exp/template/html/css.go:113: n = (n << 4) | (int(c - '0'))
If you lifted the "n <<= 4" outside the switch, these could be written with
fewer parens:
n |= int(c) - '0'

http://codereview.appspot.com/4968058/diff/8003/src/pkg/exp/template/html/css...
src/pkg/exp/template/html/css.go:244: if c < 0x80 && isCSSNmchar(int(c)) {
Is the c < 0x80 guard because CSS is limited to ASCII only or is it because you
are iterating over bytes instead of runes?

http://codereview.appspot.com/4968058/diff/8003/src/pkg/exp/template/html/css...
File src/pkg/exp/template/html/css_test.go (right):

http://codereview.appspot.com/4968058/diff/8003/src/pkg/exp/template/html/css...
src/pkg/exp/template/html/css_test.go:76: {`\`, ``},
Can you add a test case for `foo\`?

http://codereview.appspot.com/4968058/diff/8003/src/pkg/exp/template/html/css...
src/pkg/exp/template/html/css_test.go:94: {`The \3c
i\3equick\3c/i\3e,\d\A\3cspan style=\27 color:brown\27\3e brown\3c/span\3e  fox
jumps\2028over the \3c canine class=\22lazy\22 \3e dog\3c/canine\3e`,
Since this struct literal spans multiple lines, I'd format it as:
{
    `The \3c etc`,
    "The <i> etc",
},

http://codereview.appspot.com/4968058/diff/8003/src/pkg/exp/template/html/css...
src/pkg/exp/template/html/css_test.go:166: 
I'd nix the blank line.

http://codereview.appspot.com/4968058/diff/8003/src/pkg/exp/template/html/esc...
File src/pkg/exp/template/html/escape.go (right):

http://codereview.appspot.com/4968058/diff/8003/src/pkg/exp/template/html/esc...
src/pkg/exp/template/html/escape.go:78: case stateURL, stateCSSDqStr,
stateCSSSqStr,
I think it'd be clearer if the case was all on one line.

http://codereview.appspot.com/4968058/diff/8003/src/pkg/exp/template/html/esc...
src/pkg/exp/template/html/escape.go:608: 
As these three sections make one big comment, I'd make this line a "\t//"
instead of a blank line, and similarly at line 611.

http://codereview.appspot.com/4968058/diff/8003/src/pkg/exp/template/html/htm...
File src/pkg/exp/template/html/html_test.go (right):

http://codereview.appspot.com/4968058/diff/8003/src/pkg/exp/template/html/htm...
src/pkg/exp/template/html/html_test.go:37: 
I'd nix the blank line.

http://codereview.appspot.com/4968058/diff/8003/src/pkg/exp/template/html/url...
File src/pkg/exp/template/html/url_test.go (right):

http://codereview.appspot.com/4968058/diff/8003/src/pkg/exp/template/html/url...
src/pkg/exp/template/html/url_test.go:84: t.Errorf("%s once:
want\n\t%q\ngot\n\t%q", test.name, test.escaped, s)
Drop the "once"?

[MikeSamuel, 2011-09-06 04:28:16]

http://codereview.appspot.com/4968058/diff/21001/src/pkg/exp/template/html/cs...
File src/pkg/exp/template/html/css.go (right):

http://codereview.appspot.com/4968058/diff/21001/src/pkg/exp/template/html/cs...
src/pkg/exp/template/html/css.go:56: // decodeCSS decodes CSS3 escapes given a
sequence of stringchars.
On 2011/09/06 01:54:08, nigeltao wrote:
> On 2011/09/02 17:07:43, MikeSamuel wrote:
> > On 2011/09/02 09:11:37, nigeltao wrote:
> > > What's a stringchar?
> > 
> > http://www.w3.org/TR/css3-syntax/#SUBTOK-stringchar
> 
> Can you incorporate that link into the doc comment? Future maintainers might
> also not know what a stringchar is.

Done.

http://codereview.appspot.com/4968058/diff/21001/src/pkg/exp/template/html/ht...
File src/pkg/exp/template/html/html.go (right):

http://codereview.appspot.com/4968058/diff/21001/src/pkg/exp/template/html/ht...
src/pkg/exp/template/html/html.go:86: b.WriteString("&#x")
On 2011/09/06 01:54:08, nigeltao wrote:
> On 2011/09/02 17:07:43, MikeSamuel wrote:
> > On 2011/09/02 09:11:37, nigeltao wrote:
> > > fmt.Fprintf(b, "&#x%04x;", r)
> > > 
> > > Similarly in url.go.
> > 
> > Done.
> > 
> > I saw the below in template/funcs.go
> > 
> >     // TODO(dsymonds): Do this without fmt?
> >     fmt.Fprintf(w, "\\u%04X", rune)
> > 
> > and thought that format strings were frowned upon for this sort of thing.
> > 
> > Changing urlProcessor to use 
> >     fmt.Fprintf(&b, "%%%02x", c)
> > instead of
> >     b.WriteByte('%')
> >     b.WriteByte("0123456789abcdef"[c>>4])
> >     b.WriteByte("0123456789abcdef"[c&0xf])
> > causes the benchmarks to take treble the time.
> > 
> > html.BenchmarkURLEscaper          200000              8377 ns/op
> > html.BenchmarkURLNormalizer       500000              6453 ns/op
> > 
> > vs
> > 
> > html.BenchmarkURLEscaper         1000000              2553 ns/op
> > html.BenchmarkURLNormalizer      1000000              1852 ns/op
> 
> Since the benchmarks show 3x, I'm happy to go back to what you had before.

Done.

http://codereview.appspot.com/4968058/diff/8003/src/pkg/exp/template/html/css.go
File src/pkg/exp/template/html/css.go (right):

http://codereview.appspot.com/4968058/diff/8003/src/pkg/exp/template/html/css...
src/pkg/exp/template/html/css.go:33: return
bytes.HasSuffix(bytes.ToLower(b[len(b)-len(kw):]), kw)
On 2011/09/06 01:54:08, nigeltao wrote:
> Can you use bytes.Equal instead of bytes.HasSuffix?
> 
> Also, it might read better if you take the "b[len(b)-len(kw):]" out of here
and
> put
> b = b[len(b)-len(kw):]
> inside the if block above? It might help to start the function with:
> i := len(b) - len(kw)
> 
> Finally, if kw is going to be constant (and not untrusted input), it might be
> more appropriate for kw to be a string instead of a []byte. The final line
could
> be:
> return string(bytes.ToLower(b)) == kw
> 
> The compiler might not currently optimize away the []byte to string
conversion,
> but it should be able to.

Done.

http://codereview.appspot.com/4968058/diff/8003/src/pkg/exp/template/html/css...
src/pkg/exp/template/html/css.go:69: copy(b[len(b):len(b)+i], s[:i])
On 2011/09/06 01:54:08, nigeltao wrote:
> This could be:
> b, s = append(b, s[:i]...), s[i:]

Done.

http://codereview.appspot.com/4968058/diff/8003/src/pkg/exp/template/html/css...
src/pkg/exp/template/html/css.go:95: copy(b[len(b):len(b)+n], s[1:])
On 2011/09/06 01:54:08, nigeltao wrote:
> This could be:
> b, s = append(b, s[1:1+n]...), s[1+n:]

Done.

http://codereview.appspot.com/4968058/diff/8003/src/pkg/exp/template/html/css...
src/pkg/exp/template/html/css.go:113: n = (n << 4) | (int(c - '0'))
On 2011/09/06 01:54:08, nigeltao wrote:
> If you lifted the "n <<= 4" outside the switch, these could be written with
> fewer parens:
> n |= int(c) - '0'

Done.

http://codereview.appspot.com/4968058/diff/8003/src/pkg/exp/template/html/css...
src/pkg/exp/template/html/css.go:244: if c < 0x80 && isCSSNmchar(int(c)) {
On 2011/09/06 01:54:08, nigeltao wrote:
> Is the c < 0x80 guard because CSS is limited to ASCII only or is it because
you
> are iterating over bytes instead of runes?

Normally I'd never blacklist, but browser vendors have largely agreed that
JavaScript in CSS was a horribly bad idea and have moved away from allowing
that.  IE 7 was the last to allow expression(...) so this is only to guard
against JS in CSS on older but still widespread browsers.

The loop is over runes.
I'm trying to prevent any mechanisms for embedding non-standard syntactic
constructs to break a keyword..  All CSS keywords are ASCII only so I'm
collecting only ASCII nmchars because I don't trust browsers not to ignore
combining marks, bidi-embeddings, and non-standard syntactic constructs.

http://codereview.appspot.com/4968058/diff/8003/src/pkg/exp/template/html/css...
File src/pkg/exp/template/html/css_test.go (right):

http://codereview.appspot.com/4968058/diff/8003/src/pkg/exp/template/html/css...
src/pkg/exp/template/html/css_test.go:76: {`\`, ``},
On 2011/09/06 01:54:08, nigeltao wrote:
> Can you add a test case for `foo\`?

Done.

http://codereview.appspot.com/4968058/diff/8003/src/pkg/exp/template/html/css...
src/pkg/exp/template/html/css_test.go:94: {`The \3c
i\3equick\3c/i\3e,\d\A\3cspan style=\27 color:brown\27\3e brown\3c/span\3e  fox
jumps\2028over the \3c canine class=\22lazy\22 \3e dog\3c/canine\3e`,
On 2011/09/06 01:54:08, nigeltao wrote:
> Since this struct literal spans multiple lines, I'd format it as:
> {
>     `The \3c etc`,
>     "The <i> etc",
> },

Done.

http://codereview.appspot.com/4968058/diff/8003/src/pkg/exp/template/html/css...
src/pkg/exp/template/html/css_test.go:166: 
On 2011/09/06 01:54:08, nigeltao wrote:
> I'd nix the blank line.

Done.

http://codereview.appspot.com/4968058/diff/8003/src/pkg/exp/template/html/esc...
File src/pkg/exp/template/html/escape.go (right):

http://codereview.appspot.com/4968058/diff/8003/src/pkg/exp/template/html/esc...
src/pkg/exp/template/html/escape.go:78: case stateURL, stateCSSDqStr,
stateCSSSqStr,
On 2011/09/06 01:54:08, nigeltao wrote:
> I think it'd be clearer if the case was all on one line.

Done.

http://codereview.appspot.com/4968058/diff/8003/src/pkg/exp/template/html/esc...
src/pkg/exp/template/html/escape.go:608: 
On 2011/09/06 01:54:08, nigeltao wrote:
> As these three sections make one big comment, I'd make this line a "\t//"
> instead of a blank line, and similarly at line 611.

Done.

http://codereview.appspot.com/4968058/diff/8003/src/pkg/exp/template/html/htm...
File src/pkg/exp/template/html/html_test.go (right):

http://codereview.appspot.com/4968058/diff/8003/src/pkg/exp/template/html/htm...
src/pkg/exp/template/html/html_test.go:37: 
On 2011/09/06 01:54:08, nigeltao wrote:
> I'd nix the blank line.

Done.

http://codereview.appspot.com/4968058/diff/8003/src/pkg/exp/template/html/url...
File src/pkg/exp/template/html/url_test.go (right):

http://codereview.appspot.com/4968058/diff/8003/src/pkg/exp/template/html/url...
src/pkg/exp/template/html/url_test.go:84: t.Errorf("%s once:
want\n\t%q\ngot\n\t%q", test.name, test.escaped, s)
On 2011/09/06 01:54:08, nigeltao wrote:
> Drop the "once"?

Done.

[nigeltao, 2011-09-08 21:18:26]

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

exp/template/html: autoescape actions in HTML style attributes.

This does not wire up <style> elements as that is pending support
for raw text content in CL http://codereview.appspot.com/4964045/

This CL allows actions to appear in contexts like

selectors:        {{.Tag}}{{.Class}}{{.Id}}
property names:   border-{{.BidiLeadingEdge}}
property values:  color: {{.Color}}
strings:          font-family: "{{font-name}}"
URL strings:      background: "/foo?image={{.ImgQuery}}"
URL literals:     background: url("{{.Image}}")

but disallows actions inside CSS comments and disallows
embedding of JS in CSS entirely.

It is based on the CSS3 lexical grammar with affordances for
common browser extensions including line comments.

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

Committer: Nigel Tao <nigeltao@golang.org>