BugChomper utility - rewrite in Go

BugChomper utility - rewrite in Go

BUG=skia:

Committed: http://code.google.com/p/skia/source/detail?r=14715

[borenet, 2014-05-07 18:18:05]

First attempt at Go!

https://codereview.chromium.org/274693002/diff/1/tools/bug_chomper/static/thi...
File tools/bug_chomper/static/third_party/jquery-2.1.1.min.js (right):

https://codereview.chromium.org/274693002/diff/1/tools/bug_chomper/static/thi...
tools/bug_chomper/static/third_party/jquery-2.1.1.min.js:1: /*! jQuery v2.1.1 |
(c) 2005, 2014 jQuery Foundation, Inc. | jquery.org/license */
Not modified.

https://codereview.chromium.org/274693002/diff/1/tools/bug_chomper/static/thi...
File tools/bug_chomper/static/third_party/jquery.tablednd.js (right):

https://codereview.chromium.org/274693002/diff/1/tools/bug_chomper/static/thi...
tools/bug_chomper/static/third_party/jquery.tablednd.js:1: /**
Not modified.

[borenet, 2014-05-07 18:20:43]

Running (for now) at http://nautilus.cnc.corp.google.com:8000/

In addition to rewriting in Go, this version has a few improvements over the
original:
- Uses OAuth2 instead of AuthSub
- Uses HTTPS (not working remotely for me, but works locally)
- Uses real HTML templates instead of dumb string replacement.
- Adds CSS for slightly more pretty.

[borenet, 2014-05-07 18:27:05]

On 2014/05/07 18:20:43, borenet wrote:
> Running (for now) at http://nautilus.cnc.corp.google.com:8000/
> 
> In addition to rewriting in Go, this version has a few improvements over the
> original:
> - Uses OAuth2 instead of AuthSub
> - Uses HTTPS (not working remotely for me, but works locally)
> - Uses real HTML templates instead of dumb string replacement.
> - Adds CSS for slightly more pretty.

Ah!  Indeed, I've hard-coded my email address and forgotten to remove it.

[borenet, 2014-05-07 18:27:51]

On 2014/05/07 18:27:05, borenet wrote:
> On 2014/05/07 18:20:43, borenet wrote:
> > Running (for now) at http://nautilus.cnc.corp.google.com:8000/
> > 
> > In addition to rewriting in Go, this version has a few improvements over the
> > original:
> > - Uses OAuth2 instead of AuthSub
> > - Uses HTTPS (not working remotely for me, but works locally)
> > - Uses real HTML templates instead of dumb string replacement.
> > - Adds CSS for slightly more pretty.
> 
> Ah!  Indeed, I've hard-coded my email address and forgotten to remove it.

Any user who has rights to edit bugs can edit anybody's bugs, so it's not that
you're logged in as me, it's just retrieving my bugs instead of yours.

[jcgregorio, 2014-05-07 19:32:25]

High level structural comments.

https://codereview.chromium.org/274693002/diff/1/tools/bug_chomper/src/issue_...
File tools/bug_chomper/src/issue_tracker/issue_tracker.go (right):

https://codereview.chromium.org/274693002/diff/1/tools/bug_chomper/src/issue_...
tools/bug_chomper/src/issue_tracker/issue_tracker.go:49: return nil, err
For cases where you return errors like this you should use Errorf to add context
to the error:

   return nil, fmt.Errorf("failed to read OAuth config file: %s", err)

Don't begin the error message with upper case, this allows the errors to nest
context cleanly.

https://codereview.chromium.org/274693002/diff/1/tools/bug_chomper/src/issue_...
tools/bug_chomper/src/issue_tracker/issue_tracker.go:83: func (i *Issue) URL()
string {
Doesn't need to be *Issue, just Issue, since you aren't modifying i.

https://codereview.chromium.org/274693002/diff/1/tools/bug_chomper/src/server...
File tools/bug_chomper/src/server/server.go (right):

https://codereview.chromium.org/274693002/diff/1/tools/bug_chomper/src/server...
tools/bug_chomper/src/server/server.go:43: var contentTypesByExtension =
map[string]string{
Is there a reason you didn't use http://golang.org/pkg/mime/#TypeByExtension ?

https://codereview.chromium.org/274693002/diff/1/tools/bug_chomper/src/server...
tools/bug_chomper/src/server/server.go:63: // sessionStates contains the state
for all sessions. It's leaky because
It also goes away when the server restarts. This will actually cause problems in
the long run because you only get a refresh token the first time you do the
OAuth dance, after that you only get an access token, which means everyone will
have to go through the OAuth dance every time they visit the site, but more
problematic, the site will only work for 1 hour, after which the access token
expires and because there's no refresh token your calls will either fail, or if
you detect the situation, they have to do the OAuth dance again.

https://codereview.chromium.org/274693002/diff/1/tools/bug_chomper/src/server...
tools/bug_chomper/src/server/server.go:214: err =
issueTracker.UpgradeCode(code[0])
This shouldn't be part of a regular page, i.e. the page that you redirect to for
the code exchange should be its own landing page that you bounce through on your
way back. See /oauth2callback on https://gist.github.com/border/3579615. There
are security issues with not using a separate page.

https://codereview.chromium.org/274693002/diff/1/tools/bug_chomper/src/server...
tools/bug_chomper/src/server/server.go:317: func handle(resp
http.ResponseWriter, req *http.Request) {
Use w and r as the variable names here:

  func handle(w http.ResponseWriter, r *http.Request) {

Here and in all funcs that take http.ResponseWriter, *http.Request.

https://codereview.chromium.org/274693002/diff/1/tools/bug_chomper/src/server...
tools/bug_chomper/src/server/server.go:327: }
Just serve this off a subdir, i.e. do this in main():


   http.Handle("/res/", http.FileServer(http.Dir("./static")))

https://codereview.chromium.org/274693002/diff/1/tools/bug_chomper/static/not...
File tools/bug_chomper/static/notfound.html (right):

https://codereview.chromium.org/274693002/diff/1/tools/bug_chomper/static/not...
tools/bug_chomper/static/notfound.html:2: <head>
Is this file used?

https://codereview.chromium.org/274693002/diff/1/tools/bug_chomper/static/thi...
File tools/bug_chomper/static/third_party/jquery-2.1.1.min.js (right):

https://codereview.chromium.org/274693002/diff/1/tools/bug_chomper/static/thi...
tools/bug_chomper/static/third_party/jquery-2.1.1.min.js:1: /*! jQuery v2.1.1 |
(c) 2005, 2014 jQuery Foundation, Inc. | jquery.org/license */
Use the Google CDN for this: 
https://developers.google.com/speed/libraries/devguide

[borenet, 2014-05-07 19:32:32]

Correctly obtaining the user email at patch set 2.

[borenet, 2014-05-07 22:27:37]

Uploaded patch set 3 which addresses comments, except for the session/auth
timeout and server state issue.

https://codereview.chromium.org/274693002/diff/1/tools/bug_chomper/src/issue_...
File tools/bug_chomper/src/issue_tracker/issue_tracker.go (right):

https://codereview.chromium.org/274693002/diff/1/tools/bug_chomper/src/issue_...
tools/bug_chomper/src/issue_tracker/issue_tracker.go:49: return nil, err
On 2014/05/07 19:32:25, jcgregorio wrote:
> For cases where you return errors like this you should use Errorf to add
context
> to the error:
> 
>    return nil, fmt.Errorf("failed to read OAuth config file: %s", err)
> 
> Don't begin the error message with upper case, this allows the errors to nest
> context cleanly.

Done.

https://codereview.chromium.org/274693002/diff/1/tools/bug_chomper/src/issue_...
tools/bug_chomper/src/issue_tracker/issue_tracker.go:83: func (i *Issue) URL()
string {
On 2014/05/07 19:32:25, jcgregorio wrote:
> Doesn't need to be *Issue, just Issue, since you aren't modifying i.

Done, but I thought this performed a copy if it's not a pointer?

https://codereview.chromium.org/274693002/diff/1/tools/bug_chomper/src/server...
File tools/bug_chomper/src/server/server.go (right):

https://codereview.chromium.org/274693002/diff/1/tools/bug_chomper/src/server...
tools/bug_chomper/src/server/server.go:43: var contentTypesByExtension =
map[string]string{
On 2014/05/07 19:32:25, jcgregorio wrote:
> Is there a reason you didn't use http://golang.org/pkg/mime/#TypeByExtension ?

Didn't know about it!

https://codereview.chromium.org/274693002/diff/1/tools/bug_chomper/src/server...
tools/bug_chomper/src/server/server.go:63: // sessionStates contains the state
for all sessions. It's leaky because
On 2014/05/07 19:32:25, jcgregorio wrote:
> It also goes away when the server restarts. This will actually cause problems
in
> the long run because you only get a refresh token the first time you do the
> OAuth dance, after that you only get an access token, which means everyone
will
> have to go through the OAuth dance every time they visit the site, but more
> problematic, the site will only work for 1 hour, after which the access token
> expires and because there's no refresh token your calls will either fail, or
if
> you detect the situation, they have to do the OAuth dance again.

So:
- (de)serialize session states on startup/shutdown
- Add offline access to OAuth2 config (done) OR delete all expired sessions
- Detect and delete long-expired sessions
?

https://codereview.chromium.org/274693002/diff/1/tools/bug_chomper/src/server...
tools/bug_chomper/src/server/server.go:214: err =
issueTracker.UpgradeCode(code[0])
On 2014/05/07 19:32:25, jcgregorio wrote:
> This shouldn't be part of a regular page, i.e. the page that you redirect to
for
> the code exchange should be its own landing page that you bounce through on
your
> way back. See /oauth2callback on https://gist.github.com/border/3579615. There
> are security issues with not using a separate page.
> 

Done.

https://codereview.chromium.org/274693002/diff/1/tools/bug_chomper/src/server...
tools/bug_chomper/src/server/server.go:317: func handle(resp
http.ResponseWriter, req *http.Request) {
On 2014/05/07 19:32:25, jcgregorio wrote:
> Use w and r as the variable names here:
> 
>   func handle(w http.ResponseWriter, r *http.Request) {
> 
> Here and in all funcs that take http.ResponseWriter, *http.Request.

Done.  Is this just convention?  To me it seems easier to tell what "resp" and
"req" are than "w" and "r".

https://codereview.chromium.org/274693002/diff/1/tools/bug_chomper/src/server...
tools/bug_chomper/src/server/server.go:327: }
On 2014/05/07 19:32:25, jcgregorio wrote:
> Just serve this off a subdir, i.e. do this in main():
> 
> 
>    http.Handle("/res/", http.FileServer(http.Dir("./static")))

Done.

https://codereview.chromium.org/274693002/diff/1/tools/bug_chomper/static/not...
File tools/bug_chomper/static/notfound.html (right):

https://codereview.chromium.org/274693002/diff/1/tools/bug_chomper/static/not...
tools/bug_chomper/static/notfound.html:2: <head>
On 2014/05/07 19:32:25, jcgregorio wrote:
> Is this file used?

Not any more! Removed.

https://codereview.chromium.org/274693002/diff/1/tools/bug_chomper/static/thi...
File tools/bug_chomper/static/third_party/jquery-2.1.1.min.js (right):

https://codereview.chromium.org/274693002/diff/1/tools/bug_chomper/static/thi...
tools/bug_chomper/static/third_party/jquery-2.1.1.min.js:1: /*! jQuery v2.1.1 |
(c) 2005, 2014 jQuery Foundation, Inc. | jquery.org/license */
On 2014/05/07 19:32:25, jcgregorio wrote:
> Use the Google CDN for this: 
> https://developers.google.com/speed/libraries/devguide

Done.

https://codereview.chromium.org/274693002/diff/40001/tools/bug_chomper/src/is...
File tools/bug_chomper/src/issue_tracker/issue_tracker.go (right):

https://codereview.chromium.org/274693002/diff/40001/tools/bug_chomper/src/is...
tools/bug_chomper/src/issue_tracker/issue_tracker.go:47: labelUnchanged = iota
Changed for clarity.

https://codereview.chromium.org/274693002/diff/40001/tools/bug_chomper/src/is...
tools/bug_chomper/src/issue_tracker/issue_tracker.go:78: AccessType:  
"offline",
Not sure I like this.  I think I'd rather just have sessions expire and have to
re-auth.

[jcgregorio, 2014-05-08 15:11:56]

https://codereview.chromium.org/274693002/diff/1/tools/bug_chomper/src/issue_...
File tools/bug_chomper/src/issue_tracker/issue_tracker.go (right):

https://codereview.chromium.org/274693002/diff/1/tools/bug_chomper/src/issue_...
tools/bug_chomper/src/issue_tracker/issue_tracker.go:83: func (i *Issue) URL()
string {
That's for arguments, in the case of the 

On 2014/05/07 22:27:37, borenet wrote:
> On 2014/05/07 19:32:25, jcgregorio wrote:
> > Doesn't need to be *Issue, just Issue, since you aren't modifying i.
> 
> Done, but I thought this performed a copy if it's not a pointer?

That's for arguments, it's slightly different in the case of the receiver, using
(i T) is similar to declaring a function const in C++, it can't modify the
underlying receiver, while declaring it (i *T) means that the method can modify
the receiver. Note that in Go the receiver can be more general than a C++
class/struct, it just needs to be a type.

https://codereview.chromium.org/274693002/diff/1/tools/bug_chomper/src/server...
File tools/bug_chomper/src/server/server.go (right):

https://codereview.chromium.org/274693002/diff/1/tools/bug_chomper/src/server...
tools/bug_chomper/src/server/server.go:63: // sessionStates contains the state
for all sessions. It's leaky because
Again, the choices depend on if this is going to run publicly or just locally.
For a local run, just keep the info in memory, don't worry about saving it, and
use access_type online.

If this was going to be a public service another way to run would be to encrypt
and store the session state in a cookie, thus avoiding having any persistent
storage.

Actually encrypt and store the session state in a cookie might radically
simplify your code, since you now have session management in server.go and auth
stuff in issue_tracker.go, that change could consolidate it all into session
cookie handling. I've done this once before, I think I used
http://www.gorillatoolkit.org/pkg/securecookie to handle the details.


On 2014/05/07 22:27:37, borenet wrote:
> On 2014/05/07 19:32:25, jcgregorio wrote:
> > It also goes away when the server restarts. This will actually cause
problems
> in
> > the long run because you only get a refresh token the first time you do the
> > OAuth dance, after that you only get an access token, which means everyone
> will
> > have to go through the OAuth dance every time they visit the site, but more
> > problematic, the site will only work for 1 hour, after which the access
token
> > expires and because there's no refresh token your calls will either fail, or
> if
> > you detect the situation, they have to do the OAuth dance again.
> 
> So:
> - (de)serialize session states on startup/shutdown
> - Add offline access to OAuth2 config (done) OR delete all expired sessions
> - Detect and delete long-expired sessions
> ?

https://codereview.chromium.org/274693002/diff/1/tools/bug_chomper/src/server...
tools/bug_chomper/src/server/server.go:317: func handle(resp
http.ResponseWriter, req *http.Request) {
Yes, it's a convention.

On 2014/05/07 22:27:37, borenet wrote:
> On 2014/05/07 19:32:25, jcgregorio wrote:
> > Use w and r as the variable names here:
> > 
> >   func handle(w http.ResponseWriter, r *http.Request) {
> > 
> > Here and in all funcs that take http.ResponseWriter, *http.Request.
> 
> Done.  Is this just convention?  To me it seems easier to tell what "resp" and
> "req" are than "w" and "r".

https://codereview.chromium.org/274693002/diff/40001/tools/bug_chomper/src/is...
File tools/bug_chomper/src/issue_tracker/issue_tracker.go (right):

https://codereview.chromium.org/274693002/diff/40001/tools/bug_chomper/src/is...
tools/bug_chomper/src/issue_tracker/issue_tracker.go:47: labelUnchanged = iota
No need to repeat iota, see http://golang.org/doc/effective_go.html#constants

On 2014/05/07 22:27:38, borenet wrote:
> Changed for clarity.

https://codereview.chromium.org/274693002/diff/40001/tools/bug_chomper/src/is...
tools/bug_chomper/src/issue_tracker/issue_tracker.go:78: AccessType:  
"offline",
That's reasonable given the tool, so "online" would be better.

On 2014/05/07 22:27:38, borenet wrote:
> Not sure I like this.  I think I'd rather just have sessions expire and have
to
> re-auth.

https://codereview.chromium.org/274693002/diff/40001/tools/bug_chomper/src/is...
tools/bug_chomper/src/issue_tracker/issue_tracker.go:126: return
it.oauthConfig.AuthCodeURL(redirectURL)
I didn't realize how bad the goauth2 library is, in practice if you were doing
this on a public server you should have XSRF protection built into the system.
Since that's not the case I don't think you need to write it here, but here's
the code for future reference:


https://code.google.com/p/google-api-python-client/source/browse/oauth2client...

Maybe add a NOTE here about adding XSRF protection if this is ever to be run on
a public server?

https://codereview.chromium.org/274693002/diff/40001/tools/bug_chomper/src/is...
tools/bug_chomper/src/issue_tracker/issue_tracker.go:131: func (it
*IssueTracker) IsAuthenticated() bool {
it IssueTracker

Check all instances, if the call doesn't modify 'it' then it can be 'it
IssueTracker' and not 'it *IssueTracker'

https://codereview.chromium.org/274693002/diff/40001/tools/bug_chomper/src/is...
tools/bug_chomper/src/issue_tracker/issue_tracker.go:154: errFmt,
errors.New("User is not authenticated!"))
  fmt.Errorf(errFmt, errors.New(

can be more cleanly done as:

  fmt.Errorf(errFmt, "User is not authenticated")

ftm.Errorf is just fmt.Printf with the result wrapped in an error.

Here and all other instances where you use errors.New.

https://codereview.chromium.org/274693002/diff/40001/tools/bug_chomper/src/is...
tools/bug_chomper/src/issue_tracker/issue_tracker.go:179: errFmt :=
fmt.Sprintf("error retrieving issue %d: %s", id)
I don't think this works the way you want it to, see:

  http://play.golang.org/p/NkskTi5f18

https://codereview.chromium.org/274693002/diff/40001/tools/bug_chomper/src/is...
tools/bug_chomper/src/issue_tracker/issue_tracker.go:200: }
if err := json.Unmarshal(body, &issue); err != nil {
  return nil, fmt.Errorf(errFmt, err)
}

Look for this in all the cases where the func returns just an error.

https://codereview.chromium.org/274693002/diff/40001/tools/bug_chomper/src/is...
tools/bug_chomper/src/issue_tracker/issue_tracker.go:268: postData.Content =
comment
 }{
  Content: comment,
}

[borenet, 2014-05-09 15:40:11]

Uploaded patch set 4.

https://codereview.chromium.org/274693002/diff/40001/tools/bug_chomper/src/is...
File tools/bug_chomper/src/issue_tracker/issue_tracker.go (right):

https://codereview.chromium.org/274693002/diff/40001/tools/bug_chomper/src/is...
tools/bug_chomper/src/issue_tracker/issue_tracker.go:47: labelUnchanged = iota
On 2014/05/08 15:11:57, jcgregorio wrote:
> No need to repeat iota, see http://golang.org/doc/effective_go.html#constants
> 
> On 2014/05/07 22:27:38, borenet wrote:
> > Changed for clarity.
> 

Done.

https://codereview.chromium.org/274693002/diff/40001/tools/bug_chomper/src/is...
tools/bug_chomper/src/issue_tracker/issue_tracker.go:78: AccessType:  
"offline",
On 2014/05/08 15:11:57, jcgregorio wrote:
> That's reasonable given the tool, so "online" would be better.
> 
> On 2014/05/07 22:27:38, borenet wrote:
> > Not sure I like this.  I think I'd rather just have sessions expire and have
> to
> > re-auth.
> 

"offline" is the zero-value, so I just removed this line.

https://codereview.chromium.org/274693002/diff/40001/tools/bug_chomper/src/is...
tools/bug_chomper/src/issue_tracker/issue_tracker.go:126: return
it.oauthConfig.AuthCodeURL(redirectURL)
On 2014/05/08 15:11:57, jcgregorio wrote:
> I didn't realize how bad the goauth2 library is, in practice if you were doing
> this on a public server you should have XSRF protection built into the system.
> Since that's not the case I don't think you need to write it here, but here's
> the code for future reference:
> 
> 
>
https://code.google.com/p/google-api-python-client/source/browse/oauth2client...
> 
> Maybe add a NOTE here about adding XSRF protection if this is ever to be run
on
> a public server?

So my intention was to make this something which *could* run on a public server
(AppEngine, with the other skia-tree-status apps?), though maybe it'd be better
to run it only locally.  I guess we should decide one way or another, because
it'll be annoying to have to generate certs to use HTTPS and obtain the OAuth
config info when you're running locally.

Added a comment for now.

https://codereview.chromium.org/274693002/diff/40001/tools/bug_chomper/src/is...
tools/bug_chomper/src/issue_tracker/issue_tracker.go:131: func (it
*IssueTracker) IsAuthenticated() bool {
On 2014/05/08 15:11:57, jcgregorio wrote:
> it IssueTracker
> 
> Check all instances, if the call doesn't modify 'it' then it can be 'it
> IssueTracker' and not 'it *IssueTracker'

Done.

https://codereview.chromium.org/274693002/diff/40001/tools/bug_chomper/src/is...
tools/bug_chomper/src/issue_tracker/issue_tracker.go:154: errFmt,
errors.New("User is not authenticated!"))
On 2014/05/08 15:11:57, jcgregorio wrote:
>   fmt.Errorf(errFmt, errors.New(
> 
> can be more cleanly done as:
> 
>   fmt.Errorf(errFmt, "User is not authenticated")
> 
> ftm.Errorf is just fmt.Printf with the result wrapped in an error.
> 
> Here and all other instances where you use errors.New.

Done.

https://codereview.chromium.org/274693002/diff/40001/tools/bug_chomper/src/is...
tools/bug_chomper/src/issue_tracker/issue_tracker.go:179: errFmt :=
fmt.Sprintf("error retrieving issue %d: %s", id)
On 2014/05/08 15:11:57, jcgregorio wrote:
> I don't think this works the way you want it to, see:
> 
>   http://play.golang.org/p/NkskTi5f18

Modified it in a slightly dumb way.

https://codereview.chromium.org/274693002/diff/40001/tools/bug_chomper/src/is...
tools/bug_chomper/src/issue_tracker/issue_tracker.go:200: }
On 2014/05/08 15:11:57, jcgregorio wrote:
> if err := json.Unmarshal(body, &issue); err != nil {
>   return nil, fmt.Errorf(errFmt, err)
> }
> 
> Look for this in all the cases where the func returns just an error.

Done.

https://codereview.chromium.org/274693002/diff/40001/tools/bug_chomper/src/is...
tools/bug_chomper/src/issue_tracker/issue_tracker.go:268: postData.Content =
comment
On 2014/05/08 15:11:57, jcgregorio wrote:
>  }{
>   Content: comment,
> }

Done.

[epoger, 2014-05-09 15:56:46]

removing useless dinosaur from CC list

[borenet, 2014-05-12 20:05:51]

Uploaded patch set 5.

https://codereview.chromium.org/274693002/diff/40001/tools/bug_chomper/src/is...
File tools/bug_chomper/src/issue_tracker/issue_tracker.go (right):

https://codereview.chromium.org/274693002/diff/40001/tools/bug_chomper/src/is...
tools/bug_chomper/src/issue_tracker/issue_tracker.go:78: AccessType:  
"offline",
On 2014/05/09 15:40:11, borenet wrote:
> On 2014/05/08 15:11:57, jcgregorio wrote:
> > That's reasonable given the tool, so "online" would be better.
> > 
> > On 2014/05/07 22:27:38, borenet wrote:
> > > Not sure I like this.  I think I'd rather just have sessions expire and
have
> > to
> > > re-auth.
> > 
> 
> "offline" is the zero-value, so I just removed this line.

I meant "online" here.

[jcgregorio, 2014-05-13 16:28:12]

https://codereview.chromium.org/274693002/diff/80001/tools/bug_chomper/src/se...
File tools/bug_chomper/src/server/server.go (right):

https://codereview.chromium.org/274693002/diff/80001/tools/bug_chomper/src/se...
tools/bug_chomper/src/server/server.go:278: if err := makeBugChomperPage(w, r);
err != nil {
I think the code would get simpler by not returning an error from
makeBugChomperPage and submitData, but instead create an reportError func that
sets that status code, and logs the error, see:

See:
https://skia.googlesource.com/skia/+/master/experimental/webtry/webtry.go#318

https://codereview.chromium.org/274693002/diff/80001/tools/bug_chomper/src/se...
tools/bug_chomper/src/server/server.go:322: // handle is the top-level handler
function for all HTTP requests coming to
Comment no longer correct.

https://codereview.chromium.org/274693002/diff/80001/tools/bug_chomper/templa...
File tools/bug_chomper/templates/bug_chomper.html (right):

https://codereview.chromium.org/274693002/diff/80001/tools/bug_chomper/templa...
tools/bug_chomper/templates/bug_chomper.html:3: <title>{{ .Title }}</title>
Convention is to not put spaces, i.e. {{.Title}}.

Here and many other places.

https://codereview.chromium.org/274693002/diff/80001/tools/bug_chomper/templa...
tools/bug_chomper/templates/bug_chomper.html:17: edited[bug_id] =
JSON.parse(JSON.stringify(issues[bug_id]));
Shouldn't JSON.parse(JSON.stringify be a NOP?

[borenet, 2014-05-13 17:43:20]

Uploaded patch set 6.

https://codereview.chromium.org/274693002/diff/80001/tools/bug_chomper/src/se...
File tools/bug_chomper/src/server/server.go (right):

https://codereview.chromium.org/274693002/diff/80001/tools/bug_chomper/src/se...
tools/bug_chomper/src/server/server.go:278: if err := makeBugChomperPage(w, r);
err != nil {
On 2014/05/13 16:28:12, jcgregorio wrote:
> I think the code would get simpler by not returning an error from
> makeBugChomperPage and submitData, but instead create an reportError func that
> sets that status code, and logs the error, see:
> 
> See:
> https://skia.googlesource.com/skia/+/master/experimental/webtry/webtry.go#318

Done.

https://codereview.chromium.org/274693002/diff/80001/tools/bug_chomper/templa...
File tools/bug_chomper/templates/bug_chomper.html (right):

https://codereview.chromium.org/274693002/diff/80001/tools/bug_chomper/templa...
tools/bug_chomper/templates/bug_chomper.html:3: <title>{{ .Title }}</title>
On 2014/05/13 16:28:12, jcgregorio wrote:
> Convention is to not put spaces, i.e. {{.Title}}.
> 
> Here and many other places.

Done.

https://codereview.chromium.org/274693002/diff/80001/tools/bug_chomper/templa...
tools/bug_chomper/templates/bug_chomper.html:17: edited[bug_id] =
JSON.parse(JSON.stringify(issues[bug_id]));
On 2014/05/13 16:28:12, jcgregorio wrote:
> Shouldn't JSON.parse(JSON.stringify be a NOP?

This is effectively a deep copy.

[jcgregorio, 2014-05-13 17:50:17]

LGTM except for two nits.

https://codereview.chromium.org/274693002/diff/100001/tools/bug_chomper/src/s...
File tools/bug_chomper/src/server/server.go (right):

https://codereview.chromium.org/274693002/diff/100001/tools/bug_chomper/src/s...
tools/bug_chomper/src/server/server.go:92: func makeSession(
nit, keep this one line.

https://codereview.chromium.org/274693002/diff/100001/tools/bug_chomper/src/s...
tools/bug_chomper/src/server/server.go:117: func getSession(
nit, keep this one line.

[borenet, 2014-05-13 17:57:07]

Uploaded patch set 7.

https://codereview.chromium.org/274693002/diff/100001/tools/bug_chomper/src/s...
File tools/bug_chomper/src/server/server.go (right):

https://codereview.chromium.org/274693002/diff/100001/tools/bug_chomper/src/s...
tools/bug_chomper/src/server/server.go:92: func makeSession(
On 2014/05/13 17:50:17, jcgregorio wrote:
> nit, keep this one line.

Done here and elsewhere.

[borenet, 2014-05-13 17:57:17]

The CQ bit was checked by borenet@google.com

[commit-bot: I haz the power, 2014-05-13 17:57:29]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/borenet@google.com/274693002/120001

[commit-bot: I haz the power, 2014-05-13 18:03:57]

Change committed as 14715