Milo: Console improvements

milo/frontend/view_console.go

Index: milo/frontend/view_console.go
diff --git a/milo/frontend/view_console.go b/milo/frontend/view_console.go
index 595139531f699de21edf84ab68703b369ea84ba8..ed42c0c9102f4918db5703b968a0776eb019ef32 100644
--- a/milo/frontend/view_console.go
+++ b/milo/frontend/view_console.go
@@ -15,10 +15,12 @@
 package frontend
 
 import (
+	"bytes"
 	"encoding/hex"
 	"fmt"
 	"html/template"
 	"net/http"
+	"strings"
 
 	"golang.org/x/net/context"
 
@@ -49,33 +51,53 @@ func getConsoleDef(c context.Context, project, name string) (*common.Console, er
 	return cs, nil
 }
 
-func console(c context.Context, project, name string) (*resp.Console, error) {
+// shortname calculates a short name (3 char max long name) out of a full name
+// by splitting on delimiters and taking the first letter of each "word".
+// name is expected to be a builderID, which is module/<bucket or master>/buildername
+func shortname(name string) string {
+	builderNameComp := strings.SplitN(name, "/", 3)
+	if len(builderNameComp) == 3 {
+		name = builderNameComp[2]
+	}
+	tokens := strings.FieldsFunc(name, func(r rune) bool {
+		switch r {
+		case '_', '-', ' ':
+			return true
+		}
+		return false
+	})
+	numLetters := len(tokens)
+	if numLetters > 3 {
+		numLetters = 3
+	}
+	short := ""
+	for i := 0; i < numLetters; i++ {
+		if len(tokens[i]) > 0 {

[dnj, 2017/08/03 01:02:59]

Note that if you have five tokens, "1--2-3-4", this will result in "12", not
"123", since the skipped empty token will still count as one of the
"numLetters".

WDYT about this technique:
pos := 0
for i := 0; i < len(tokens) && pos < 3; i++ {
  if tok := tokens[i]; len(tok) > 0 {
    tokens[pos] = tok
    pos++
  }
}
return strings.Join(tokens[:pos], "")

[Ryan Tseng, 2017/08/03 14:06:16]

Actually, playing around with it for a bit, it seems like fieldsfunc is smart
enough to discard empty tokens:
https://play.golang.org/p/MEtpn7O7hO

So the empty string check is actually unnecessary

+			short += string(tokens[i][0])
+		}
+	}
+	return strings.ToLower(short)
+}
+
+func console(c context.Context, project, name string, limit int) (*resp.Console, error) {
 	tStart := clock.Now(c)
 	def, err := getConsoleDef(c, project, name)
 	if err != nil {
 		return nil, err
 	}
-	commitInfo, err := git.GetHistory(c, def.RepoURL, def.Ref, 25)
+	commitInfo, err := git.GetHistory(c, def.RepoURL, def.Ref, limit)
 	if err != nil {
 		return nil, err
 	}
 	tGitiles := clock.Now(c)
 	logging.Debugf(c, "Loading commits took %s.", tGitiles.Sub(tStart))
 
-	builderNames := make([]string, len(def.Builders))
-	builders := make([]resp.BuilderRef, len(def.Builders))
-	for i, b := range def.Builders {
-		builderNames[i] = b
-		builders[i].Name = b
-		_, _, builders[i].ShortName, _ = buildsource.BuilderID(b).Split()
-		// TODO(hinoka): Add Categories back in.
-	}
-
 	commitNames := make([]string, len(commitInfo.Commits))
 	for i, commit := range commitInfo.Commits {
 		commitNames[i] = hex.EncodeToString(commit.Hash)
 	}
-	rows, err := buildsource.GetConsoleRows(c, project, def, commitNames, builderNames)
+
+	rows, err := buildsource.GetConsoleRows(c, project, def, commitNames, def.Builders)
 	tConsole := clock.Now(c)
 	logging.Debugf(c, "Loading the console took a total of %s.", tConsole.Sub(tGitiles))
 	if err != nil {
@@ -83,8 +105,9 @@ func console(c context.Context, project, name string) (*resp.Console, error) {
 	}
 
 	ccb := make([]resp.CommitBuild, len(commitInfo.Commits))
+	builderRefs := make([]resp.BuilderRef, len(def.Builders))
 	for row, commit := range commitInfo.Commits {
-		ccb[row].Build = make([]*model.BuildSummary, len(builders))
+		ccb[row].Build = make([]*model.BuildSummary, len(def.Builders))
 		ccb[row].Commit = resp.Commit{
 			AuthorName:  commit.AuthorName,
 			AuthorEmail: commit.AuthorEmail,
@@ -95,8 +118,18 @@ func console(c context.Context, project, name string) (*resp.Console, error) {
 			Revision:    resp.NewLink(commitNames[row], def.RepoURL+"/+/"+commitNames[row]),
 		}
 
-		for col, b := range builders {
-			name := buildsource.BuilderID(b.Name)
+		for col, b := range def.Builders {
+			meta := def.BuilderMetas[col]
+			short := meta.ShortName
+			if short == "" {
+				short = shortname(b)
+			}
+			builderRefs[col] = resp.BuilderRef{
+				Name:      b,
+				Category:  strings.Split(meta.Category, "|"),
+				ShortName: short,
+			}
+			name := buildsource.BuilderID(b)
 			if summaries := rows[row].Builds[name]; len(summaries) > 0 {
 				ccb[row].Build[col] = summaries[0]
 			}
@@ -106,7 +139,7 @@ func console(c context.Context, project, name string) (*resp.Console, error) {
 	return &resp.Console{
 		Name:       def.ID,
 		Commit:     ccb,
-		BuilderRef: builders,
+		BuilderRef: builderRefs,
 	}, nil
 }
 
@@ -127,9 +160,14 @@ func (c consoleRenderer) Header() template.HTML {
 		}
 	}
 
-	result := ""
+	var buf bytes.Buffer
+	must := func(_ int, err error) {
+		if err != nil {
+			panic(err)
+		}
+	}
 	for row := 0; row < depth; row++ {
-		result += "<tr><th></th><th></th>"
+		must(buf.WriteString("<tr><th></th><th></th>"))
 		// "" is the first two nodes, " " is an empty node.
 		current := ""
 		colspan := 0
@@ -143,25 +181,32 @@ func (c consoleRenderer) Header() template.HTML {
 			}
 			if s != current || current == " " {
 				if current != "" || current == " " {
-					result += fmt.Sprintf(`<th colspan="%d">%s</th>`, colspan, current)
+					must(fmt.Fprintf(&buf, `<th colspan="%d">%s</th>`, colspan, current))
 					colspan = 0
 				}
 				current = s
 			}
 		}
 		if colspan != 0 {
-			result += fmt.Sprintf(`<th colspan="%d">%s</th>`, colspan, current)
+			must(fmt.Fprintf(&buf, `<th colspan="%d">%s</th>`, colspan, current))
 		}
-		result += "</tr>"
+		must(buf.WriteString("</tr>"))
 	}
 
 	// Last row: The actual builder shortnames.
-	result += "<tr><th></th><th></th>"
+	must(buf.WriteString("<tr><th></th><th></th>"))
 	for _, br := range c.BuilderRef {
-		result += fmt.Sprintf("<th>%s</th>", br.ShortName)
+		_, _, builderName, err := buildsource.BuilderID(br.Name).Split()
+		if err != nil {
+			must(fmt.Fprintf(&buf, "<th><a title=\"%s\">ERR</a></th>", err.Error()))

[dnj, 2017/08/03 01:02:59]

Nit: should probably escape the error string in case it has some weird
characters in it: https://golang.org/pkg/html/template/#HTMLEscapeString

[Ryan Tseng, 2017/08/03 14:06:16]

Done.

+		} else {
+			must(fmt.Fprintf(
+				&buf, "<th><a href=\"/%s\" title=\"%s\">%s</a></th>",
+				br.Name, builderName, br.ShortName))

[dnj, 2017/08/03 01:02:59]

(*probably* safe not to escape these, although a malicious builder name could
totally XSS you, builder names are defined by trusted actors.

[Ryan Tseng, 2017/08/03 14:06:16]

Better safe than sorry.

+		}
 	}
-	result += "</tr>"
-	return template.HTML(result)
+	must(buf.WriteString("</tr>"))
+	return template.HTML(buf.String())
 }
 
 func (c consoleRenderer) BuilderLink(bs *model.BuildSummary) (*resp.Link, error) {
@@ -180,8 +225,15 @@ func ConsoleHandler(c *router.Context) {
 		return
 	}
 	name := c.Params.ByName("name")
+	limit := 25
+	if tLimit := GetLimit(c.Request, -1); tLimit >= 0 {
+		limit = tLimit
+	}
+	if limit > 1000 {
+		limit = 1000
+	}
 
-	result, err := console(c.Context, project, name)
+	result, err := console(c.Context, project, name, limit)
 	if err != nil {
 		ErrorHandler(c, err)
 		return