Start a top-down rewrite of Garden-O-Matic in Polymer

Start a top-down rewrite of Garden-O-Matic in Polymer

In previous CLs, we've been working up from the bottom of the widget hierarchy
to rewrite Garden-O-Matic in Polymer. This CL moves this project forward by
starting from the top of the widget hierarchy. When these two efforts meet in
the middle, we'll be done!

This CL is mostly about fleshing out the structure of the components. We'll
need to iterate on the visual style of each component in future CLs. I've added
dependencies on Polymer's core and paper elements because the core elements are
super useful and the paper elements are super pretty.

The top-down version is hosted in gom2.html to avoid breaking the working
product. Currently, gom2 shows correctly grouped sets of failing tests and the
regression ranges for each group (albeit without any fancy styling).

NOTRY=true

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=177172

[ojan, 2014-06-28 16:29:53]

Michael, Doug, FYI since you're working in this code as well.

[ojan, 2014-06-28 16:50:10]

This looks good to me except for the lack of tests. Please at least add tests
for all the components that execute script.

In my ideal world we'd even add tests for the static components as well. Just a
simple test that verifies that we actually get an element with the right basic
structure. For example, ct-commit-list could verify that it has the right number
of ct-commit's in it as a sanity check that there are no typos in the MDV bits.

While we're adding new files and rewriting it all, may as well use this as an
opportunity to change names. How about sheriff-o-matic? Then we also don't need
to give it a temporary name.

I see you did two-space indent. That's in preparation for moving it somewhere
non-blink? I hate to say it, but should we also do 80 char lines? Or should we
hope that we won't require that for html content?

https://codereview.chromium.org/345853005/diff/1/Tools/GardeningServer/ui/ct-...
File Tools/GardeningServer/ui/ct-failure-grouper.html (right):

https://codereview.chromium.org/345853005/diff/1/Tools/GardeningServer/ui/ct-...
Tools/GardeningServer/ui/ct-failure-grouper.html:31: this.groups[i] =
map[key].sort(this.failureComparator.bind(this));
Why the bind?

[abarth-chromium, 2014-06-28 17:52:37]

https://codereview.chromium.org/345853005/diff/1/Tools/GardeningServer/ui/ct-...
File Tools/GardeningServer/ui/ct-failure-grouper.html (right):

https://codereview.chromium.org/345853005/diff/1/Tools/GardeningServer/ui/ct-...
Tools/GardeningServer/ui/ct-failure-grouper.html:31: this.groups[i] =
map[key].sort(this.failureComparator.bind(this));
Just so I don't get bitten by not having the right |this| later.

[abarth-chromium, 2014-06-28 17:54:21]

On 2014/06/28 at 16:50:10, ojan wrote:
> This looks good to me except for the lack of tests. Please at least add tests
for all the components that execute script.

Ok.

> In my ideal world we'd even add tests for the static components as well. Just
a simple test that verifies that we actually get an element with the right basic
structure. For example, ct-commit-list could verify that it has the right number
of ct-commit's in it as a sanity check that there are no typos in the MDV bits.

Ok.

> While we're adding new files and rewriting it all, may as well use this as an
opportunity to change names. How about sheriff-o-matic? Then we also don't need
to give it a temporary name.

We can rename things later.  That's much easier that rewriting the entire UI.

> I see you did two-space indent. That's in preparation for moving it somewhere
non-blink? I hate to say it, but should we also do 80 char lines? Or should we
hope that we won't require that for html content?

We can worry about these problems in the future.

[ojan, 2014-06-28 19:00:28]

> > While we're adding new files and rewriting it all, may as well use this as
an opportunity to change names. How about sheriff-o-matic? Then we also don't
need to give it a temporary name.
> 
> We can rename things later.  That's much easier that rewriting the entire UI.

All I had in mind was making the <title>Sheriff-O-Matic</title>, renaming
gom2.html to sheriff-o-matic.html and renaming ct-garden-o-matic.html to
ct-sheriff-o-matic.html. We can of course that later if you want. I don't feel
strongly. Just seemed natural to do it now while we're creating the new files.

> > I see you did two-space indent. That's in preparation for moving it
somewhere non-blink? I hate to say it, but should we also do 80 char lines? Or
should we hope that we won't require that for html content?
> 
> We can worry about these problems in the future.

Yeah, I'm not very worried about it, but it seemed a strange choice to use a
style that is neither the Blink style nor the Chromium style.

[abarth-chromium, 2014-06-28 19:34:25]

On 2014/06/28 at 19:00:28, ojan wrote:
> > > While we're adding new files and rewriting it all, may as well use this as
an opportunity to change names. How about sheriff-o-matic? Then we also don't
need to give it a temporary name.
> > 
> > We can rename things later.  That's much easier that rewriting the entire
UI.
> 
> All I had in mind was making the <title>Sheriff-O-Matic</title>, renaming
gom2.html to sheriff-o-matic.html and renaming ct-garden-o-matic.html to
ct-sheriff-o-matic.html. We can of course that later if you want. I don't feel
strongly. Just seemed natural to do it now while we're creating the new files.

Done.

[abarth-chromium, 2014-06-28 20:02:26]

PTAL

I've added a couple tests.  I need to think about the right way to test elements
that use data providers.  I could go all the way down into the JS layer and use
the NetworkSimulator, but that doesn't quite seem like the right thing. 
Instead, the problem is that they're grabbing at the "model" static to get their
data.  I need to abstract the root data source somehow and then use dependency
injection...  Anyway, I'd like to defer that for a future CL.

[ojan, 2014-06-29 03:18:52]

On 2014/06/28 at 20:02:26, abarth wrote:
> I've added a couple tests.  I need to think about the right way to test
elements that use data providers.  I could go all the way down into the JS layer
and use the NetworkSimulator, but that doesn't quite seem like the right thing. 
Instead, the problem is that they're grabbing at the "model" static to get their
data.  I need to abstract the root data source somehow and then use dependency
injection...  Anyway, I'd like to defer that for a future CL.

What I had in mind was just to overwrite the relevant model.* methods with
mocks. It's brittle and not awesome, but it's something. It's true that
abstracting out the root data source is a better way to go about it. I'm just
wary whenever we add code that isn't tested. It's a slippery slope (broken glass
theory-wise) to having a large untested codebase. 

In either case, I trust you. So, lgtm as is. Although, you might want to wait
for esprehn to give this a once-over since he knows polymery-things better than
me. Of course he can look it over after it's been committed as well.

Thanks again for helping out with this.

[abarth-chromium, 2014-06-29 04:40:08]

Thanks for the thoughtful review.

For what it's worth, I agree with your viewpoint on testing this code.  It was
me who originally pushed for testing GOM.  I think the GOM tests have been
useful in a number of ways, but they've also been brittle and difficult to
maintain in other ways, particularly the tests of the old UI modules.  I think
we have an opportunity to do better here with components, but I'm not sure I
figured it all out yet.

With respect to overwriting the relevant model.* methods, I think we want to
move away from static JS objects and towards a component model.  We'll get it
all sorted out.

I guess that's all a long-winded way of saying that this CL is more like a
checkpoint rather than a complete, well-formed change.  I guess could keep it
locally and continue working, but it seemed better to land it and open
possibilities for collaboration...

[abarth-chromium, 2014-06-29 04:40:15]

The CQ bit was checked by abarth@chromium.org

[commit-bot: I haz the power, 2014-06-29 04:40:59]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/abarth@chromium.org/345853005/40001

[commit-bot: I haz the power, 2014-06-29 04:41:33]

Change committed as 177172

[ojan, 2014-06-29 04:48:25]

https://codereview.chromium.org/345853005/diff/1/Tools/GardeningServer/gom2.html
File Tools/GardeningServer/gom2.html (right):

https://codereview.chromium.org/345853005/diff/1/Tools/GardeningServer/gom2.h...
Tools/GardeningServer/gom2.html:1: <!--
This should use the new minimal copyright?

https://codereview.chromium.org/345853005/diff/40001/Tools/GardeningServer/ui...
File Tools/GardeningServer/ui/ct-unexpected-failures.html (right):

https://codereview.chromium.org/345853005/diff/40001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-unexpected-failures.html:29: <paper-toast id="toast"
text="{{analyzerStatus}}"></paper-toast>
BTW, I tried this out locally and I'm not sure the toast is the right UI for
this. If the analyzing step takes a long time and you happen to not be looking
when the toast appears, it won't be clear that the tool is stuck trying to
update. I guess we could pop the toast back up if we saw it taking a long time.

None of this is super important, but I figured I'd mention it.

I guess we really just want a feature request for paper-toast to allow you to
give it a duration of 0 or -1 or something to mean that the page will manually
close/update it.

[ojan, 2014-06-29 04:49:59]

Just saw your other comments...I agree that landing and collaborating with less
initial test-coverage is better than hacking on it locally.

[abarth-chromium, 2014-06-29 05:36:32]

On 2014/06/29 at 04:48:25, ojan wrote:
>
https://codereview.chromium.org/345853005/diff/1/Tools/GardeningServer/gom2.html
> File Tools/GardeningServer/gom2.html (right):
> 
>
https://codereview.chromium.org/345853005/diff/1/Tools/GardeningServer/gom2.h...
> Tools/GardeningServer/gom2.html:1: <!--
> This should use the new minimal copyright?

Probably.  Will fix.

>
https://codereview.chromium.org/345853005/diff/40001/Tools/GardeningServer/ui...
> File Tools/GardeningServer/ui/ct-unexpected-failures.html (right):
> 
>
https://codereview.chromium.org/345853005/diff/40001/Tools/GardeningServer/ui...
> Tools/GardeningServer/ui/ct-unexpected-failures.html:29: <paper-toast
id="toast" text="{{analyzerStatus}}"></paper-toast>
> BTW, I tried this out locally and I'm not sure the toast is the right UI for
this. If the analyzing step takes a long time and you happen to not be looking
when the toast appears, it won't be clear that the tool is stuck trying to
update. I guess we could pop the toast back up if we saw it taking a long time.
> 
> None of this is super important, but I figured I'd mention it.
> 
> I guess we really just want a feature request for paper-toast to allow you to
give it a duration of 0 or -1 or something to mean that the page will manually
close/update it.

Yeah, I think we want to keep the message open visible until its done.  Not sure
if that means we should use something aside from the toast.  Mostly I was
experimenting with the paper widgets.

[ojan, 2014-06-29 16:51:12]

https://codereview.chromium.org/345853005/diff/40001/Tools/GardeningServer/ui...
File Tools/GardeningServer/ui/ct-sheriff-o-matic.html (right):

https://codereview.chromium.org/345853005/diff/40001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-sheriff-o-matic.html:33: <paper-tabs
selected="{{selected}}">
I've been thinking more about the UI we use for results. I'm starting to think
that tabs is not the best. Why don't we just expand the details inline in the
failure summary directly instead of taking you to a new tab?

[abarth-chromium, 2014-06-29 20:06:32]

On 2014/06/29 at 16:51:12, ojan wrote:
> I've been thinking more about the UI we use for results. I'm starting to think
that tabs is not the best. Why don't we just expand the details inline in the
failure summary directly instead of taking you to a new tab?

Once we finish converting the existing UI to Polymer, we can consider changing
the UI.  We need to keep the individual steps scoped if we're going to make
progress.

[esprehn, 2014-07-08 16:25:06]

Lots of random nits, lgtm though.

https://codereview.chromium.org/345853005/diff/40001/Tools/GardeningServer/ui...
File Tools/GardeningServer/ui/ct-commit-data.html (right):

https://codereview.chromium.org/345853005/diff/40001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-commit-data.html:14: firstChanged: function() {
You can use computed properties for this. Make one that depends on both
properties and then add a change listener.

computed: {
  state: "[first, last]",
}

https://codereview.chromium.org/345853005/diff/40001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-commit-data.html:22: update_: function() {
stateChanged: function() {

https://codereview.chromium.org/345853005/diff/40001/Tools/GardeningServer/ui...
File Tools/GardeningServer/ui/ct-commit-list.html (right):

https://codereview.chromium.org/345853005/diff/40001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-commit-list.html:10: <polymer-element
name="ct-commit-list" attributes="first last" noscript>
Missing commitData

https://codereview.chromium.org/345853005/diff/40001/Tools/GardeningServer/ui...
File Tools/GardeningServer/ui/ct-failure-analyzer.html (right):

https://codereview.chromium.org/345853005/diff/40001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-failure-analyzer.html:20:
model.analyzeUnexpectedFailures(function(failureAnalysis, total) {
return model.analyzeUnexpectedFailures(...)

that way the top level Promise will also reject if the thing explodes.

https://codereview.chromium.org/345853005/diff/40001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-failure-analyzer.html:27: }.bind(this));
You drop errors on the floor which is going to make this super hard to debug.
They'll just get swallowed. You need:

.catch(function(e) { 
  console.log(e);
});

https://codereview.chromium.org/345853005/diff/40001/Tools/GardeningServer/ui...
File Tools/GardeningServer/ui/ct-failure-grouper.html (right):

https://codereview.chromium.org/345853005/diff/40001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-failure-grouper.html:31: this.groups[i] =
map[key].sort(this.failureComparator.bind(this));
binding the comparator just makes this code slow, it's stateless so I'd avoid
binding it. You might even make it a free function instead.

(function() {
  function compare(a, b) {
    return ...;
  }

  Polymer({
    ...
  });
})();

See that pattern here:
https://github.com/esprehn/chromium-codereview/blob/master/ui/components/cr-k...

https://codereview.chromium.org/345853005/diff/40001/Tools/GardeningServer/ui...
File Tools/GardeningServer/ui/ct-failure-stream.html (right):

https://codereview.chromium.org/345853005/diff/40001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-failure-stream.html:12: <hr>
<hr> sucks, I'd have just used divs, same thing with the <br> below.

https://codereview.chromium.org/345853005/diff/40001/Tools/GardeningServer/ui...
File Tools/GardeningServer/ui/ct-sheriff-o-matic.html (right):

https://codereview.chromium.org/345853005/diff/40001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-sheriff-o-matic.html:24: <polymer-element
name="ct-sheriff-o-matic">
You almost certainly want attributes="selected" I think?

https://codereview.chromium.org/345853005/diff/40001/Tools/GardeningServer/ui...
File Tools/GardeningServer/ui/ct-unexpected-failures.html (right):

https://codereview.chromium.org/345853005/diff/40001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-unexpected-failures.html:16: paper-toast {
You probably meant to add :host { display: block; } to most of these elements.
The default of inline is rarely what oyu want.

https://codereview.chromium.org/345853005/diff/40001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-unexpected-failures.html:28: <ct-failure-stream
groups="{{failureGroups}}"></ct-failure-stream>
Some of these seem to be data sources and some are UI controls, it's nice if you
group the sources together at the top or bottom and add an extra new line to
separate them.