code review 6945067: go.talks/present: abstract websocket code from playground UI

go.talks/present: use Playground API when served by App Engine

[adg, 2012-12-17 00:20:07]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go.talks

[adg, 2012-12-17 02:20:21]

https://codereview.appspot.com/6945067/diff/6005/present/static/playground.js
File present/static/playground.js (right):

https://codereview.appspot.com/6945067/diff/6005/present/static/playground.js...
present/static/playground.js:1: // Copyright 2012 The Go Authors. All rights
reserved.
This is just copying playground.js from the go-playground repo.
The existing playground.js was renamed to play.js.

https://codereview.appspot.com/6945067/diff/6005/present/static/socket.js
File present/static/socket.js (right):

https://codereview.appspot.com/6945067/diff/6005/present/static/socket.js#new...
present/static/socket.js:1: // Copyright 2012 The Go Authors. All rights
reserved.
This code was moved out of play.js (née playground.js).

[dsymonds, 2012-12-17 02:59:35]

https://codereview.appspot.com/6945067/diff/6006/present/appengine.go
File present/appengine.go (right):

https://codereview.appspot.com/6945067/diff/6006/present/appengine.go#newcode20
present/appengine.go:20: const runUrl = "http://golang.org/compile"
runURL

https://codereview.appspot.com/6945067/diff/6006/present/appengine.go#newcode43
present/appengine.go:43: c.Errorf("making POST request:", err)
I'd probably hoist the error logging into compile, and just return annotated
errors down here.

https://codereview.appspot.com/6945067/diff/6006/present/play.go
File present/play.go (right):

https://codereview.appspot.com/6945067/diff/6006/present/play.go#newcode22
present/play.go:22: fmt.Fprintf(&buf, `document.write(unescape("%%3Cscript
src='%s' type='text/javascript'%%3E%%3C/script%%3E"));`, p)
ugh, this is awful. There should really be some proper escaping going on around
here. TODO?

https://codereview.appspot.com/6945067/diff/6006/present/static/play.js
File present/static/play.js (right):

https://codereview.appspot.com/6945067/diff/6006/present/static/play.js#newco...
present/static/play.js:39: if (stopFunc)
add braces

https://codereview.appspot.com/6945067/diff/6006/present/static/play.js#newco...
present/static/play.js:40: stopFunc();
should this also set stopFunc to null?

https://codereview.appspot.com/6945067/diff/6006/present/static/play.js#newco...
present/static/play.js:99: console.log("No playground transport available.");
is this going to be visible enough? should it be an alert?

https://codereview.appspot.com/6945067/diff/6006/present/static/playground.js
File present/static/playground.js (right):

https://codereview.appspot.com/6945067/diff/6006/present/static/playground.js...
present/static/playground.js:11: while (r ) {
s/r /r/

[adg, 2012-12-17 03:13:29]

https://codereview.appspot.com/6945067/diff/6006/present/appengine.go
File present/appengine.go (right):

https://codereview.appspot.com/6945067/diff/6006/present/appengine.go#newcode20
present/appengine.go:20: const runUrl = "http://golang.org/compile"
On 2012/12/17 02:59:35, dsymonds wrote:
> runURL

Done.

https://codereview.appspot.com/6945067/diff/6006/present/appengine.go#newcode43
present/appengine.go:43: c.Errorf("making POST request:", err)
On 2012/12/17 02:59:35, dsymonds wrote:
> I'd probably hoist the error logging into compile, and just return annotated
> errors down here.

Done.

https://codereview.appspot.com/6945067/diff/6006/present/play.go
File present/play.go (right):

https://codereview.appspot.com/6945067/diff/6006/present/play.go#newcode22
present/play.go:22: fmt.Fprintf(&buf, `document.write(unescape("%%3Cscript
src='%s' type='text/javascript'%%3E%%3C/script%%3E"));`, p)
On 2012/12/17 02:59:35, dsymonds wrote:
> ugh, this is awful. There should really be some proper escaping going on
around
> here. TODO?

Added a todo. It's not *that* bad.

https://codereview.appspot.com/6945067/diff/6006/present/static/play.js
File present/static/play.js (right):

https://codereview.appspot.com/6945067/diff/6006/present/static/play.js#newco...
present/static/play.js:39: if (stopFunc)
On 2012/12/17 02:59:35, dsymonds wrote:
> add braces

Done.

https://codereview.appspot.com/6945067/diff/6006/present/static/play.js#newco...
present/static/play.js:40: stopFunc();
On 2012/12/17 02:59:35, dsymonds wrote:
> should this also set stopFunc to null?

nah

https://codereview.appspot.com/6945067/diff/6006/present/static/play.js#newco...
present/static/play.js:99: console.log("No playground transport available.");
On 2012/12/17 02:59:35, dsymonds wrote:
> is this going to be visible enough? should it be an alert?

It's really just for me. If I'm like "why isn't this working?" the console will
tell me. End users will just see a graceful degradation to no playground
support.

https://codereview.appspot.com/6945067/diff/6006/present/static/playground.js
File present/static/playground.js (right):

https://codereview.appspot.com/6945067/diff/6006/present/static/playground.js...
present/static/playground.js:11: while (r ) {
On 2012/12/17 02:59:35, dsymonds wrote:
> s/r /r/

Done.

[dsymonds, 2012-12-17 03:24:49]

LGTM

https://codereview.appspot.com/6945067/diff/8024/present/appengine.go
File present/appengine.go (right):

https://codereview.appspot.com/6945067/diff/8024/present/appengine.go#newcode39
present/appengine.go:39: b.WriteTo(w)
io.Copy(w, b) is probably the most efficient; I can never remember who sniffs
for what, except I know io.Copy sniffs everything.

[adg, 2012-12-17 03:30:51]

*** Submitted as
https://code.google.com/p/go/source/detail?r=23a6d0a397e8&repo=talks ***

go.talks/present: use Playground API when served by App Engine

R=golang-dev, dsymonds
CC=golang-dev
https://codereview.appspot.com/6945067

https://codereview.appspot.com/6945067/diff/8024/present/appengine.go
File present/appengine.go (right):

https://codereview.appspot.com/6945067/diff/8024/present/appengine.go#newcode39
present/appengine.go:39: b.WriteTo(w)
On 2012/12/17 03:24:49, dsymonds wrote:
> io.Copy(w, b) is probably the most efficient; I can never remember who sniffs
> for what, except I know io.Copy sniffs everything.

Done.