Milo Refactor: Remove theme support

Milo Refactor: Remove theme support and bootstrap theme

This CL:
* Removes the bootstrap theme (It hasn't been maintained for a while)(
* Removes theming support
* Renames settings to common
* Renames settings/settings.go to middleware.go
* Moves the expectation and template folders so they're not nested by theme name.
* Remove the ThemedHandler type and replace it with normal handlers that matches the rest of luci-go.
* Remove the TestData type and replace them with functions.

BUG=698846
Review-Url: https://codereview.chromium.org/2748073006
Committed: https://github.com/luci/luci-go/commit/e0e9f21852f8e48d79169e16d7000731422cec8d

[Ryan Tseng, 2017-03-15 18:13:29]

The CQ bit was checked by hinoka@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-15 18:13:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-15 18:19:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-15 18:19:48]

Dry run: This issue passed the CQ dry run.

[Ryan Tseng, 2017-03-15 18:30:57]

The CQ bit was checked by hinoka@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-15 18:31:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-15 18:40:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-15 18:40:37]

Dry run: This issue passed the CQ dry run.

[hinoka, 2017-03-15 18:50:09]

Description was changed from

==========
Milo Refactor: Remove theme support

BUG=
==========

to

==========
Milo Refactor: Remove theme support and bootstrap theme

This CL:
* Removes the bootstrap theme (It hasn't been maintained for a while)(
* Removes theming support
* Renames settings/settings.go to middleware.go
* Moves the expectation and template folders so they're not nested by theme
name.
* Remove the ThemedHandler type and replace it with normal handlers that matches
the rest of luci-go.  This also means template names are specified in frontend/
instead.
* Remove the TestData type and replace them with functions.

BUG=
==========

[hinoka, 2017-03-15 18:50:09]

hinoka@chromium.org changed reviewers:
+ nodir@chromium.org

[hinoka, 2017-03-15 18:50:17]

Description was changed from

==========
Milo Refactor: Remove theme support and bootstrap theme

This CL:
* Removes the bootstrap theme (It hasn't been maintained for a while)(
* Removes theming support
* Renames settings/settings.go to middleware.go
* Moves the expectation and template folders so they're not nested by theme
name.
* Remove the ThemedHandler type and replace it with normal handlers that matches
the rest of luci-go.  This also means template names are specified in frontend/
instead.
* Remove the TestData type and replace them with functions.

BUG=
==========

to

==========
Milo Refactor: Remove theme support and bootstrap theme

This CL:
* Removes the bootstrap theme (It hasn't been maintained for a while)(
* Removes theming support
* Renames settings/settings.go to middleware.go
* Moves the expectation and template folders so they're not nested by theme
name.
* Remove the ThemedHandler type and replace it with normal handlers that matches
the rest of luci-go.  This also means template names are specified in frontend/
instead.
* Remove the TestData type and replace them with functions.

BUG=698846
==========

[hinoka, 2017-03-15 18:51:15]

hinoka@chromium.org changed reviewers:
+ hinoka@chromium.org

[hinoka, 2017-03-15 18:51:16]

ptal.

https://codereview.chromium.org/2748073006/diff/20001/milo/api/proto/pb.disco...
File milo/api/proto/pb.discovery.go (right):

https://codereview.chromium.org/2748073006/diff/20001/milo/api/proto/pb.disco...
milo/api/proto/pb.discovery.go:1: // AUTOGENERATED. DO NOT EDIT.
I think this probably got regenerated due to updated proto library

https://codereview.chromium.org/2748073006/diff/20001/milo/appengine/settings...
File milo/appengine/settings/middleware.go (right):

https://codereview.chromium.org/2748073006/diff/20001/milo/appengine/settings...
milo/appengine/settings/middleware.go:1: // Copyright 2015 The LUCI Authors. All
rights reserved.
This was settings.go, but renamed and trimmed down.

[nodir, 2017-03-16 17:42:51]

https://codereview.chromium.org/2748073006/diff/20001/milo/appengine/buildbot...
File milo/appengine/buildbot/html.go (right):

https://codereview.chromium.org/2748073006/diff/20001/milo/appengine/buildbot...
milo/appengine/buildbot/html.go:21: // BuildHandler Render the buildbot build
page.
renders?

https://codereview.chromium.org/2748073006/diff/20001/milo/appengine/buildbot...
milo/appengine/buildbot/html.go:62: return args, nil
without templates, I don't see why not render template right here. This is what
other luci-go apps do, e.g.
https://cs.chromium.org/chromium/infra/go/src/github.com/luci/luci-go/schedul...

https://codereview.chromium.org/2748073006/diff/20001/milo/appengine/frontend...
File milo/appengine/frontend/milo.go (right):

https://codereview.chromium.org/2748073006/diff/20001/milo/appengine/frontend...
milo/appengine/frontend/milo.go:39: r.GET("/", basemw,
settings.Wrap(frontpageHandler, "frontpage.html"))
i don't see a reason to decouple handler functions and template names. No other
template would work for the arguments returned by the function. I'd make
handlers self-sufficient and remove the knowledge of template names from this
file by moving template names into the handler functions, see my other
suggestion in buildbot/html.go

here is how luci scheduler does this
https://cs.chromium.org/chromium/infra/go/src/github.com/luci/luci-go/schedul...

https://codereview.chromium.org/2748073006/diff/20001/milo/appengine/frontend...
File milo/appengine/frontend/milo_test.go (right):

https://codereview.chromium.org/2748073006/diff/20001/milo/appengine/frontend...
milo/appengine/frontend/milo_test.go:29: "golang.org/x/net/context"
nit: this import should go before imports of "our" packages

https://codereview.chromium.org/2748073006/diff/20001/milo/appengine/settings...
File milo/appengine/settings/middleware.go (right):

https://codereview.chromium.org/2748073006/diff/20001/milo/appengine/settings...
milo/appengine/settings/middleware.go:1: // Copyright 2015 The LUCI Authors. All
rights reserved.
why is this generic file in settings package? i'd expect common functions to be
a in a common package

https://codereview.chromium.org/2748073006/diff/20001/milo/appengine/settings...
milo/appengine/settings/middleware.go:22: "golang.org/x/net/context"
put 3p imports before 1p, in a separate section
also applies to httprouter

https://codereview.chromium.org/2748073006/diff/20001/milo/appengine/settings...
milo/appengine/settings/middleware.go:71: m =
m.Extend(templates.WithTemplates(GetTemplateBundle()))
i think it is more natural to inject http request into the context before this
line. In this case you don't need WithRequest function

https://codereview.chromium.org/2748073006/diff/20001/milo/appengine/settings...
milo/appengine/settings/middleware.go:83: args, err := h(c.Context, c.Request,
c.Params)
why not pass c itself?

https://codereview.chromium.org/2748073006/diff/20001/milo/appengine/settings...
milo/appengine/settings/middleware.go:89: if merr, ok := err.(miloerror.Error);
ok {
without templates, I don't see why do we need miloerror. Why not write http
responses directly.

[hinoka, 2017-03-16 21:04:06]

no patchset, but replying to existing comments.

The last use case for laying it out this way is to allow for default error
templates.

https://codereview.chromium.org/2748073006/diff/20001/milo/appengine/settings...
File milo/appengine/settings/middleware.go (right):

https://codereview.chromium.org/2748073006/diff/20001/milo/appengine/settings...
milo/appengine/settings/middleware.go:1: // Copyright 2015 The LUCI Authors. All
rights reserved.
On 2017/03/16 17:42:51, nodir wrote:
> why is this generic file in settings package? i'd expect common functions to
be
> a in a common package

ack, splitting settings into config and common

https://codereview.chromium.org/2748073006/diff/20001/milo/appengine/settings...
milo/appengine/settings/middleware.go:71: m =
m.Extend(templates.WithTemplates(GetTemplateBundle()))
On 2017/03/16 17:42:51, nodir wrote:
> i think it is more natural to inject http request into the context before this
> line. In this case you don't need WithRequest function

You can't though... you don't get the request object until the handler is
called, which is the only reason it's in the wrap() function.

https://codereview.chromium.org/2748073006/diff/20001/milo/appengine/settings...
milo/appengine/settings/middleware.go:83: args, err := h(c.Context, c.Request,
c.Params)
On 2017/03/16 17:42:51, nodir wrote:
> why not pass c itself?

It's a different type of context.  I think estaab's intern did this in a
refactor.

https://codereview.chromium.org/2748073006/diff/20001/milo/appengine/settings...
milo/appengine/settings/middleware.go:89: if merr, ok := err.(miloerror.Error);
ok {
On 2017/03/16 17:42:51, nodir wrote:
> without templates, I don't see why do we need miloerror. Why not write http
> responses directly.

To do default error templates? Otherwise this error handling code would need to
be replicated everywhere.

[Ryan Tseng, 2017-03-17 19:59:32]

The CQ bit was checked by hinoka@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-17 19:59:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hinoka, 2017-03-17 20:00:21]

Addressed comments.

https://codereview.chromium.org/2748073006/diff/20001/milo/appengine/buildbot...
File milo/appengine/buildbot/html.go (right):

https://codereview.chromium.org/2748073006/diff/20001/milo/appengine/buildbot...
milo/appengine/buildbot/html.go:21: // BuildHandler Render the buildbot build
page.
On 2017/03/16 17:42:51, nodir wrote:
> renders?

Now it does

https://codereview.chromium.org/2748073006/diff/20001/milo/appengine/buildbot...
milo/appengine/buildbot/html.go:62: return args, nil
On 2017/03/16 17:42:51, nodir wrote:
> without templates, I don't see why not render template right here. This is
what
> other luci-go apps do, e.g.
>
https://cs.chromium.org/chromium/infra/go/src/github.com/luci/luci-go/schedul...

Done.

https://codereview.chromium.org/2748073006/diff/20001/milo/appengine/frontend...
File milo/appengine/frontend/milo.go (right):

https://codereview.chromium.org/2748073006/diff/20001/milo/appengine/frontend...
milo/appengine/frontend/milo.go:39: r.GET("/", basemw,
settings.Wrap(frontpageHandler, "frontpage.html"))
On 2017/03/16 17:42:51, nodir wrote:
> i don't see a reason to decouple handler functions and template names. No
other
> template would work for the arguments returned by the function. I'd make
> handlers self-sufficient and remove the knowledge of template names from this
> file by moving template names into the handler functions, see my other
> suggestion in buildbot/html.go
> 
> here is how luci scheduler does this
>
https://cs.chromium.org/chromium/infra/go/src/github.com/luci/luci-go/schedul...

Done.

https://codereview.chromium.org/2748073006/diff/20001/milo/appengine/frontend...
File milo/appengine/frontend/milo_test.go (right):

https://codereview.chromium.org/2748073006/diff/20001/milo/appengine/frontend...
milo/appengine/frontend/milo_test.go:29: "golang.org/x/net/context"
On 2017/03/16 17:42:51, nodir wrote:
> nit: this import should go before imports of "our" packages

Done.

https://codereview.chromium.org/2748073006/diff/20001/milo/appengine/settings...
File milo/appengine/settings/middleware.go (right):

https://codereview.chromium.org/2748073006/diff/20001/milo/appengine/settings...
milo/appengine/settings/middleware.go:1: // Copyright 2015 The LUCI Authors. All
rights reserved.
On 2017/03/16 21:04:06, hinoka wrote:
> On 2017/03/16 17:42:51, nodir wrote:
> > why is this generic file in settings package? i'd expect common functions to
> be
> > a in a common package
> 
> ack, splitting settings into config and common

Actually theres an import loop this way, just renaming this to common

https://codereview.chromium.org/2748073006/diff/20001/milo/appengine/settings...
milo/appengine/settings/middleware.go:22: "golang.org/x/net/context"
On 2017/03/16 17:42:51, nodir wrote:
> put 3p imports before 1p, in a separate section
> also applies to httprouter

Done.

https://codereview.chromium.org/2748073006/diff/20001/milo/appengine/settings...
milo/appengine/settings/middleware.go:71: m =
m.Extend(templates.WithTemplates(GetTemplateBundle()))
On 2017/03/16 21:04:06, hinoka wrote:
> On 2017/03/16 17:42:51, nodir wrote:
> > i think it is more natural to inject http request into the context before
this
> > line. In this case you don't need WithRequest function
> 
> You can't though... you don't get the request object until the handler is
> called, which is the only reason it's in the wrap() function.

Turns out you can, changed.

https://codereview.chromium.org/2748073006/diff/20001/milo/appengine/settings...
milo/appengine/settings/middleware.go:83: args, err := h(c.Context, c.Request,
c.Params)
On 2017/03/16 21:04:06, hinoka wrote:
> On 2017/03/16 17:42:51, nodir wrote:
> > why not pass c itself?
> 
> It's a different type of context.  I think estaab's intern did this in a
> refactor.

Done.

https://codereview.chromium.org/2748073006/diff/20001/milo/appengine/settings...
milo/appengine/settings/middleware.go:89: if merr, ok := err.(miloerror.Error);
ok {
On 2017/03/16 21:04:06, hinoka wrote:
> On 2017/03/16 17:42:51, nodir wrote:
> > without templates, I don't see why do we need miloerror. Why not write http
> > responses directly.
> 
> To do default error templates? Otherwise this error handling code would need
to
> be replicated everywhere.

Changed error handling code to replicate everywhere

[hinoka, 2017-03-17 20:00:39]

Description was changed from

==========
Milo Refactor: Remove theme support and bootstrap theme

This CL:
* Removes the bootstrap theme (It hasn't been maintained for a while)(
* Removes theming support
* Renames settings/settings.go to middleware.go
* Moves the expectation and template folders so they're not nested by theme
name.
* Remove the ThemedHandler type and replace it with normal handlers that matches
the rest of luci-go.  This also means template names are specified in frontend/
instead.
* Remove the TestData type and replace them with functions.

BUG=698846
==========

to

==========
Milo Refactor: Remove theme support and bootstrap theme

This CL:
* Removes the bootstrap theme (It hasn't been maintained for a while)(
* Removes theming support
* Renames settings to common
* Renames settings/settings.go to middleware.go
* Moves the expectation and template folders so they're not nested by theme
name.
* Remove the ThemedHandler type and replace it with normal handlers that matches
the rest of luci-go.  This also means template names are specified in frontend/
instead.
* Remove the TestData type and replace them with functions.

BUG=698846
==========

[hinoka, 2017-03-17 20:00:55]

Description was changed from

==========
Milo Refactor: Remove theme support and bootstrap theme

This CL:
* Removes the bootstrap theme (It hasn't been maintained for a while)(
* Removes theming support
* Renames settings to common
* Renames settings/settings.go to middleware.go
* Moves the expectation and template folders so they're not nested by theme
name.
* Remove the ThemedHandler type and replace it with normal handlers that matches
the rest of luci-go.  This also means template names are specified in frontend/
instead.
* Remove the TestData type and replace them with functions.

BUG=698846
==========

to

==========
Milo Refactor: Remove theme support and bootstrap theme

This CL:
* Removes the bootstrap theme (It hasn't been maintained for a while)(
* Removes theming support
* Renames settings to common
* Renames settings/settings.go to middleware.go
* Moves the expectation and template folders so they're not nested by theme
name.
* Remove the ThemedHandler type and replace it with normal handlers that matches
the rest of luci-go.
* Remove the TestData type and replace them with functions.

BUG=698846
==========

[commit-bot: I haz the power, 2017-03-17 20:14:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-17 20:14:07]

Dry run: Try jobs failed on following builders:
  Luci-go Presubmit on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/34f6ac64fc374c10)

[Ryan Tseng, 2017-03-17 20:15:47]

The CQ bit was checked by hinoka@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-17 20:15:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-17 20:23:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-17 20:23:47]

Dry run: This issue passed the CQ dry run.

[nodir, 2017-03-17 20:47:58]

https://codereview.chromium.org/2748073006/diff/40001/milo/appengine/buildbot...
File milo/appengine/buildbot/html.go (right):

https://codereview.chromium.org/2748073006/diff/40001/milo/appengine/buildbot...
milo/appengine/buildbot/html.go:75: result, err := builderImpl(c.Context,
master, builder, limit)
nice, builderImpl still does not deal with response directly

https://codereview.chromium.org/2748073006/diff/40001/milo/appengine/buildbuc...
File milo/appengine/buildbucket/builder.go (right):

https://codereview.chromium.org/2748073006/diff/40001/milo/appengine/buildbuc...
milo/appengine/buildbucket/builder.go:232: server, bucket, builder,
maxCompletedBuilds := q.Server, q.Bucket, q.Builder, q.Limit
please accept builderQuery instead of *builderQuery, so that you can safely
mutate q and inline these variables

https://codereview.chromium.org/2748073006/diff/40001/milo/appengine/buildbuc...
File milo/appengine/buildbucket/html.go (right):

https://codereview.chromium.org/2748073006/diff/40001/milo/appengine/buildbuc...
milo/appengine/buildbucket/html.go:20: func validateParams(r *http.Request, p
httprouter.Params) (*builderQuery, error) {
s/validateParams/parseBuilderQuery/
because it parses the request to a builder query, and returns an error if the
input is bad

https://codereview.chromium.org/2748073006/diff/40001/milo/appengine/buildbuc...
milo/appengine/buildbucket/html.go:70: return
remove

https://codereview.chromium.org/2748073006/diff/40001/milo/appengine/common/e...
File milo/appengine/common/error.go (right):

https://codereview.chromium.org/2748073006/diff/40001/milo/appengine/common/e...
milo/appengine/common/error.go:20: return
remove

https://codereview.chromium.org/2748073006/diff/40001/milo/appengine/common/h...
File milo/appengine/common/html.go (right):

https://codereview.chromium.org/2748073006/diff/40001/milo/appengine/common/h...
milo/appengine/common/html.go:15: func ConfigsHandler(c *router.Context) {
this code is not common. It is used only in frontend package and I think it
should be there

https://codereview.chromium.org/2748073006/diff/40001/milo/appengine/common/h...
milo/appengine/common/html.go:20: "Error while getting projects : "+err.Error())
remove space after "projects" before colon?

https://codereview.chromium.org/2748073006/diff/40001/milo/appengine/common/m...
File milo/appengine/common/middleware.go (right):

https://codereview.chromium.org/2748073006/diff/40001/milo/appengine/common/m...
milo/appengine/common/middleware.go:8: "golang.org/x/net/context"
move it after stdlib imports

https://codereview.chromium.org/2748073006/diff/40001/milo/appengine/common/m...
milo/appengine/common/middleware.go:22: // getTemplateBundles is used to render
HTML templates. It provides a base args
"GetTemplateBundle"

https://codereview.chromium.org/2748073006/diff/40001/milo/appengine/common/m...
milo/appengine/common/middleware.go:22: // getTemplateBundles is used to render
HTML templates. It provides a base args
"base args" without "a"

https://codereview.chromium.org/2748073006/diff/40001/milo/appengine/common/m...
milo/appengine/common/middleware.go:40: if err != nil {
remove

https://codereview.chromium.org/2748073006/diff/40001/milo/appengine/common/m...
milo/appengine/common/middleware.go:45: "IsAnonymous": auth.CurrentIdentity(c)
== "anonymous:anonymous",
use
https://godoc.org/github.com/luci/luci-go/server/auth/identity#AnonymousIdentity

https://codereview.chromium.org/2748073006/diff/40001/milo/appengine/common/m...
milo/appengine/common/middleware.go:58: // Base returns the basic luci appengine
middlewares.
"LUCI" because it is Go

https://codereview.chromium.org/2748073006/diff/40001/milo/appengine/frontend...
File milo/appengine/frontend/milo.go (right):

https://codereview.chromium.org/2748073006/diff/40001/milo/appengine/frontend...
milo/appengine/frontend/milo.go:1: // Copyright 2015 The LUCI Authors. All
rights reserved.
file name milo is abstract, while the contents are not
maybe rename this file to handlers.go, because it defines handlers

https://codereview.chromium.org/2748073006/diff/40001/milo/appengine/frontend...
milo/appengine/frontend/milo.go:43: common.UpdateHandler)
why UpdateHandler is in common package?

https://codereview.chromium.org/2748073006/diff/40001/milo/appengine/frontend...
File milo/appengine/frontend/milo_test.go (right):

https://codereview.chromium.org/2748073006/diff/40001/milo/appengine/frontend...
milo/appengine/frontend/milo_test.go:19: .
"github.com/smartystreets/goconvey/convey"
dot imports should be in the last section, even if they are 3p

https://codereview.chromium.org/2748073006/diff/40001/milo/appengine/logdog/h...
File milo/appengine/logdog/http.go (right):

https://codereview.chromium.org/2748073006/diff/40001/milo/appengine/logdog/h...
milo/appengine/logdog/http.go:25: // AnnotationStreamHandler is a ThemedHandler
that renders a LogDog Milo
this comment is no longer correct

maybe remove L33-L39?

https://codereview.chromium.org/2748073006/diff/40001/milo/appengine/logdog/h...
milo/appengine/logdog/http.go:61: common.ErrorPage(c, http.StatusNotFound,
"Stream does not exist")
nice

https://codereview.chromium.org/2748073006/diff/40001/milo/appengine/logdog/h...
milo/appengine/logdog/http.go:69: log.WithError(err).Errorf(c.Context, "Failed
to load LogDog steram.")
typo in stream

https://codereview.chromium.org/2748073006/diff/40001/milo/appengine/logdog/h...
milo/appengine/logdog/http.go:73: }
you can do return here, so you don't have to repeat return in each switch-case

https://codereview.chromium.org/2748073006/diff/40001/milo/appengine/swarming...
File milo/appengine/swarming/html.go (right):

https://codereview.chromium.org/2748073006/diff/40001/milo/appengine/swarming...
milo/appengine/swarming/html.go:54: type Build struct {
why do we need this type?

https://codereview.chromium.org/2748073006/diff/40001/milo/appengine/swarming...
milo/appengine/swarming/html.go:94: // Render renders both the build page and
the log.
s/Render/handler
or rename func to Render

[nodir, 2017-03-17 20:49:02]

Looks much better btw

[nodir, 2017-03-17 20:50:23]

Looks much better btw

[Ryan Tseng, 2017-03-17 22:03:52]

The CQ bit was checked by hinoka@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-17 22:03:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hinoka, 2017-03-17 22:04:56]

https://codereview.chromium.org/2748073006/diff/40001/milo/appengine/buildbot...
File milo/appengine/buildbot/html.go (right):

https://codereview.chromium.org/2748073006/diff/40001/milo/appengine/buildbot...
milo/appengine/buildbot/html.go:75: result, err := builderImpl(c.Context,
master, builder, limit)
On 2017/03/17 20:47:57, nodir wrote:
> nice, builderImpl still does not deal with response directly

Ack?

https://codereview.chromium.org/2748073006/diff/40001/milo/appengine/buildbuc...
File milo/appengine/buildbucket/builder.go (right):

https://codereview.chromium.org/2748073006/diff/40001/milo/appengine/buildbuc...
milo/appengine/buildbucket/builder.go:232: server, bucket, builder,
maxCompletedBuilds := q.Server, q.Bucket, q.Builder, q.Limit
On 2017/03/17 20:47:57, nodir wrote:
> please accept builderQuery instead of *builderQuery, so that you can safely
> mutate q and inline these variables

Done.

https://codereview.chromium.org/2748073006/diff/40001/milo/appengine/buildbuc...
File milo/appengine/buildbucket/html.go (right):

https://codereview.chromium.org/2748073006/diff/40001/milo/appengine/buildbuc...
milo/appengine/buildbucket/html.go:20: func validateParams(r *http.Request, p
httprouter.Params) (*builderQuery, error) {
On 2017/03/17 20:47:57, nodir wrote:
> s/validateParams/parseBuilderQuery/
> because it parses the request to a builder query, and returns an error if the
> input is bad

Done.

https://codereview.chromium.org/2748073006/diff/40001/milo/appengine/buildbuc...
milo/appengine/buildbucket/html.go:70: return
On 2017/03/17 20:47:57, nodir wrote:
> remove

Done.

https://codereview.chromium.org/2748073006/diff/40001/milo/appengine/common/e...
File milo/appengine/common/error.go (right):

https://codereview.chromium.org/2748073006/diff/40001/milo/appengine/common/e...
milo/appengine/common/error.go:20: return
On 2017/03/17 20:47:57, nodir wrote:
> remove

Done.

https://codereview.chromium.org/2748073006/diff/40001/milo/appengine/common/h...
File milo/appengine/common/html.go (right):

https://codereview.chromium.org/2748073006/diff/40001/milo/appengine/common/h...
milo/appengine/common/html.go:15: func ConfigsHandler(c *router.Context) {
On 2017/03/17 20:47:58, nodir wrote:
> this code is not common. It is used only in frontend package and I think it
> should be there

Done.

https://codereview.chromium.org/2748073006/diff/40001/milo/appengine/common/h...
milo/appengine/common/html.go:20: "Error while getting projects : "+err.Error())
On 2017/03/17 20:47:57, nodir wrote:
> remove space after "projects" before colon?

Done.

https://codereview.chromium.org/2748073006/diff/40001/milo/appengine/common/m...
File milo/appengine/common/middleware.go (right):

https://codereview.chromium.org/2748073006/diff/40001/milo/appengine/common/m...
milo/appengine/common/middleware.go:8: "golang.org/x/net/context"
On 2017/03/17 20:47:58, nodir wrote:
> move it after stdlib imports

gofmt just did it for me.

https://codereview.chromium.org/2748073006/diff/40001/milo/appengine/common/m...
milo/appengine/common/middleware.go:22: // getTemplateBundles is used to render
HTML templates. It provides a base args
On 2017/03/17 20:47:58, nodir wrote:
> "base args" without "a"

Done.

https://codereview.chromium.org/2748073006/diff/40001/milo/appengine/common/m...
milo/appengine/common/middleware.go:40: if err != nil {
On 2017/03/17 20:47:58, nodir wrote:
> remove

Done.

https://codereview.chromium.org/2748073006/diff/40001/milo/appengine/common/m...
milo/appengine/common/middleware.go:45: "IsAnonymous": auth.CurrentIdentity(c)
== "anonymous:anonymous",
On 2017/03/17 20:47:58, nodir wrote:
> use
>
https://godoc.org/github.com/luci/luci-go/server/auth/identity#AnonymousIdentity

Done.

https://codereview.chromium.org/2748073006/diff/40001/milo/appengine/common/m...
milo/appengine/common/middleware.go:58: // Base returns the basic luci appengine
middlewares.
On 2017/03/17 20:47:58, nodir wrote:
> "LUCI" because it is Go

Done.

https://codereview.chromium.org/2748073006/diff/40001/milo/appengine/frontend...
File milo/appengine/frontend/milo.go (right):

https://codereview.chromium.org/2748073006/diff/40001/milo/appengine/frontend...
milo/appengine/frontend/milo.go:1: // Copyright 2015 The LUCI Authors. All
rights reserved.
On 2017/03/17 20:47:58, nodir wrote:
> file name milo is abstract, while the contents are not
> maybe rename this file to handlers.go, because it defines handlers

Done.

Actually i'll rename this main.go, since it is the entry point.

https://codereview.chromium.org/2748073006/diff/40001/milo/appengine/frontend...
milo/appengine/frontend/milo.go:43: common.UpdateHandler)
On 2017/03/17 20:47:58, nodir wrote:
> why UpdateHandler is in common package?

Moved

https://codereview.chromium.org/2748073006/diff/40001/milo/appengine/frontend...
File milo/appengine/frontend/milo_test.go (right):

https://codereview.chromium.org/2748073006/diff/40001/milo/appengine/frontend...
milo/appengine/frontend/milo_test.go:19: .
"github.com/smartystreets/goconvey/convey"
On 2017/03/17 20:47:58, nodir wrote:
> dot imports should be in the last section, even if they are 3p

Done.

https://codereview.chromium.org/2748073006/diff/40001/milo/appengine/logdog/h...
File milo/appengine/logdog/http.go (right):

https://codereview.chromium.org/2748073006/diff/40001/milo/appengine/logdog/h...
milo/appengine/logdog/http.go:25: // AnnotationStreamHandler is a ThemedHandler
that renders a LogDog Milo
On 2017/03/17 20:47:58, nodir wrote:
> this comment is no longer correct
> 
> maybe remove L33-L39?

Done.

Removing 33-39 is a rabbit hole :(

Fear not, I intend on removing all this in another CL.

https://codereview.chromium.org/2748073006/diff/40001/milo/appengine/logdog/h...
milo/appengine/logdog/http.go:61: common.ErrorPage(c, http.StatusNotFound,
"Stream does not exist")
On 2017/03/17 20:47:58, nodir wrote:
> nice

Acknowledged.

https://codereview.chromium.org/2748073006/diff/40001/milo/appengine/logdog/h...
milo/appengine/logdog/http.go:69: log.WithError(err).Errorf(c.Context, "Failed
to load LogDog steram.")
On 2017/03/17 20:47:58, nodir wrote:
> typo in stream

Done.

https://codereview.chromium.org/2748073006/diff/40001/milo/appengine/logdog/h...
milo/appengine/logdog/http.go:73: }
On 2017/03/17 20:47:58, nodir wrote:
> you can do return here, so you don't have to repeat return in each switch-case

Done.

https://codereview.chromium.org/2748073006/diff/40001/milo/appengine/swarming...
File milo/appengine/swarming/html.go (right):

https://codereview.chromium.org/2748073006/diff/40001/milo/appengine/swarming...
milo/appengine/swarming/html.go:54: type Build struct {
On 2017/03/17 20:47:58, nodir wrote:
> why do we need this type?

It's used as a delivery mechanism to pass the build loader around for testing
(It should live in the context instead).  I really want to get rid of it but
it's a much larger refactor.  It'll come in a followup CL

https://codereview.chromium.org/2748073006/diff/40001/milo/appengine/swarming...
milo/appengine/swarming/html.go:94: // Render renders both the build page and
the log.
On 2017/03/17 20:47:58, nodir wrote:
> s/Render/handler
> or rename func to Render

Done.

[commit-bot: I haz the power, 2017-03-17 22:12:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-17 22:12:27]

Dry run: This issue passed the CQ dry run.

[Ryan Tseng, 2017-03-17 23:59:04]

The CQ bit was checked by hinoka@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-17 23:59:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-18 00:07:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-18 00:07:04]

Dry run: Try jobs failed on following builders:
  Luci-go Mac Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/34f78796fa291210)

[hinoka, 2017-03-20 17:23:55]

monday ping.

I want this to go in before anything else.  How does it look?

[nodir, 2017-03-20 19:11:36]

lgtm

https://codereview.chromium.org/2748073006/diff/100001/milo/appengine/buildbo...
File milo/appengine/buildbot/build.go (right):

https://codereview.chromium.org/2748073006/diff/100001/milo/appengine/buildbo...
milo/appengine/buildbot/build.go:26: var errBuildNotFound = errors.New("Build
not found")
error messages in Go should start with a non-capital letter (because they are
often combined like ("higher-level reason blah blah: %s", err))

I've noticed that this is systematic, so not in this CL

https://codereview.chromium.org/2748073006/diff/100001/milo/appengine/buildbo...
File milo/appengine/buildbot/html.go (right):

https://codereview.chromium.org/2748073006/diff/100001/milo/appengine/buildbo...
milo/appengine/buildbot/html.go:44: switch err {
nice

https://codereview.chromium.org/2748073006/diff/100001/milo/appengine/fronten...
File milo/appengine/frontend/config.go (right):

https://codereview.chromium.org/2748073006/diff/100001/milo/appengine/fronten...
milo/appengine/frontend/config.go:32: func UpdateHandler(ctx *router.Context) {
(in another CL) UpdateHandler is abstract name. UpdateConfigHandler?

https://codereview.chromium.org/2748073006/diff/100001/milo/appengine/fronten...
milo/appengine/frontend/config.go:34: err := common.Update(c)
(in another CL): "common.Update" is very abstract. I think it should be
common.UpdateConfig or config.Update

[hinoka, 2017-03-20 19:16:31]

https://codereview.chromium.org/2748073006/diff/100001/milo/appengine/buildbo...
File milo/appengine/buildbot/build.go (right):

https://codereview.chromium.org/2748073006/diff/100001/milo/appengine/buildbo...
milo/appengine/buildbot/build.go:26: var errBuildNotFound = errors.New("Build
not found")
On 2017/03/20 19:11:36, nodir wrote:
> error messages in Go should start with a non-capital letter (because they are
> often combined like ("higher-level reason blah blah: %s", err))
> 
> I've noticed that this is systematic, so not in this CL

Acknowledged.

https://codereview.chromium.org/2748073006/diff/100001/milo/appengine/buildbo...
File milo/appengine/buildbot/html.go (right):

https://codereview.chromium.org/2748073006/diff/100001/milo/appengine/buildbo...
milo/appengine/buildbot/html.go:44: switch err {
On 2017/03/20 19:11:36, nodir wrote:
> nice

Acknowledged.

https://codereview.chromium.org/2748073006/diff/100001/milo/appengine/fronten...
File milo/appengine/frontend/config.go (right):

https://codereview.chromium.org/2748073006/diff/100001/milo/appengine/fronten...
milo/appengine/frontend/config.go:32: func UpdateHandler(ctx *router.Context) {
On 2017/03/20 19:11:36, nodir wrote:
> (in another CL) UpdateHandler is abstract name. UpdateConfigHandler?

Acknowledged.

https://codereview.chromium.org/2748073006/diff/100001/milo/appengine/fronten...
milo/appengine/frontend/config.go:34: err := common.Update(c)
On 2017/03/20 19:11:36, nodir wrote:
> (in another CL): "common.Update" is very abstract. I think it should be
> common.UpdateConfig or config.Update

Acknowledged.

[hinoka, 2017-03-20 19:16:38]

The CQ bit was checked by hinoka@chromium.org

[commit-bot: I haz the power, 2017-03-20 19:16:46]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-20 19:24:08]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1490037398890860,
"parent_rev": "25242b2bc72df05066ffbfcd54ef39edcaafbf04", "commit_rev":
"e0e9f21852f8e48d79169e16d7000731422cec8d"}

[commit-bot: I haz the power, 2017-03-20 19:24:12]

Description was changed from

==========
Milo Refactor: Remove theme support and bootstrap theme

This CL:
* Removes the bootstrap theme (It hasn't been maintained for a while)(
* Removes theming support
* Renames settings to common
* Renames settings/settings.go to middleware.go
* Moves the expectation and template folders so they're not nested by theme
name.
* Remove the ThemedHandler type and replace it with normal handlers that matches
the rest of luci-go.
* Remove the TestData type and replace them with functions.

BUG=698846
==========

to

==========
Milo Refactor: Remove theme support and bootstrap theme

This CL:
* Removes the bootstrap theme (It hasn't been maintained for a while)(
* Removes theming support
* Renames settings to common
* Renames settings/settings.go to middleware.go
* Moves the expectation and template folders so they're not nested by theme
name.
* Remove the ThemedHandler type and replace it with normal handlers that matches
the rest of luci-go.
* Remove the TestData type and replace them with functions.

BUG=698846

Review-Url: https://codereview.chromium.org/2748073006
Committed:
https://github.com/luci/luci-go/commit/e0e9f21852f8e48d79169e16d7000731422cec8d
==========

[commit-bot: I haz the power, 2017-03-20 19:24:14]

Committed patchset #6 (id:100001) as
https://github.com/luci/luci-go/commit/e0e9f21852f8e48d79169e16d7000731422cec8d