[sheriff-o-matic]: Basic trooper display for sheriff-o-matic.

[sheriff-o-matic]: Basic trooper display for sheriff-o-matic.

Adds trooper feed type & pulling of trooper alerts feed from trooper-o-matic.
Refactors ct-failure-card to accept different card types.
Adds failure card types for sheriff, cq-latency, cycle-time and tree-status.

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

[ojan, 2014-08-16 02:15:40]

This is a good start and roughly what I had in mind. If you'd be willing to
write tests for any of this, that'd be appreciated, but you certainly don't have
to since this code is already untested and you're not making big changes to it.

The other change I want to make to this code that you also don't have to do is
to make this a CTFailures (or CTAlerts maybe?) model class instead of a
ct-failure-analyzer polymer component.

As for where to get the data, we're discussing on the bug...but the short
version is we need integrate the data you want into
http://sheriff-o-matic.appspot.com/alerts.

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

https://codereview.chromium.org/476903004/diff/60001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-failure-analyzer.html:104: _processFailuresForGroup:
function(group, failures, annotations, tree) {
Nit: I typically annotate optional arguments with opt_tree in google3 style.

[shans, 2014-09-02 00:15:24]

On 2014/08/16 02:15:40, ojan-only-code-yellow-reviews wrote:
> This is a good start and roughly what I had in mind. If you'd be willing to
> write tests for any of this, that'd be appreciated, but you certainly don't
have
> to since this code is already untested and you're not making big changes to
it.
> 
> The other change I want to make to this code that you also don't have to do is
> to make this a CTFailures (or CTAlerts maybe?) model class instead of a
> ct-failure-analyzer polymer component.
> 
> As for where to get the data, we're discussing on the bug...but the short
> version is we need integrate the data you want into
> http://sheriff-o-matic.appspot.com/alerts.
> 
>
https://codereview.chromium.org/476903004/diff/60001/Tools/GardeningServer/ui...
> File Tools/GardeningServer/ui/ct-failure-analyzer.html (right):
> 
>
https://codereview.chromium.org/476903004/diff/60001/Tools/GardeningServer/ui...
> Tools/GardeningServer/ui/ct-failure-analyzer.html:104:
_processFailuresForGroup:
> function(group, failures, annotations, tree) {
> Nit: I typically annotate optional arguments with opt_tree in google3 style.

I think this is ready for a review. PTAL.

[ojan, 2014-09-02 02:35:12]

I left a few code comments, but first some UI comments.

1. This page should just show alerts (i.e. things that are exceeding the alert
threshold). When there are no alerts, it should show partytime.
2. I don't think we want the graphs. The text is enough IMO. Particularly, this
page will soon have dozens of alerts, so we don't want to devote this much
screen real estate to something that's sufficiently covered by the text.
3. Once we get rid of the graphs, we can simplify the card structure to just
have ct-bot-failure-card and ct-text-failure-card or something like that.
4. Instead of adding a "link" link, lets have the examine page iframe the
appropriate trooper-o-matic.appspot page.

eseidel, I CC'ed you because I know you have opinions about this page taking a
single feed off the server instead of multiple feeds. Shane, I think one way you
could do that is to modify alerts.py to inject the trooper feed into the feed
that builder_alerts is uploading to it. You can keep it in the same format it's
in now (except with filter out entries that aren't exceeding their threshold),
just put it adjacent to the alerts field (e.g. trooper_alerts). I'm OK with
doing that as a FIXME though in the name of incremental progress. 

FWIW, down the road, I'd also like to change the commit descriptions to be
injected at this point as well instead of fetching them from googlesource in the
JS code. The reason doing all the data merging on the server-side makes sense is
that it makes it so that other tools can be written to read the feed and do
things with it (e.g. track the number of alerts and send an email when it
exceeds some threshold).

https://codereview.chromium.org/476903004/diff/120001/Tools/GardeningServer/m...
File Tools/GardeningServer/model/ct-failure-group.html (right):

https://codereview.chromium.org/476903004/diff/120001/Tools/GardeningServer/m...
Tools/GardeningServer/model/ct-failure-group.html:20: function
CTTrooperFailureGroup(data, type, tree, annotation) {
Rather than using inheritance for this, in other places I've used create
methods. CTFailureGroup.createSheriffGroup and
CTFailureGroup.createTrooperGroup. Actually, even better here would be to just
have one sub-class and use composition for the different behaviors. So,
something like...

function CTFailureGroup(data, type, annotation) {...}

Where data is one of CTTrooperFailureGroupData or CTSheriffFailureGroupData,
which has all the appropriate data and methods (e.g. now CTFailureGroup wouldn't
know anything about commitLists).

https://codereview.chromium.org/476903004/diff/120001/Tools/GardeningServer/m...
File Tools/GardeningServer/model/ct-failures.html (right):

https://codereview.chromium.org/476903004/diff/120001/Tools/GardeningServer/m...
Tools/GardeningServer/model/ct-failures.html:17: this.failures = {}
This won't update right until https://codereview.chromium.org/526633002/ lands
since you don't clear the old alerts. Might want to rebase your patch on top of
that one.

https://codereview.chromium.org/476903004/diff/120001/Tools/GardeningServer/m...
Tools/GardeningServer/model/ct-failures.html:82:
net.json('http://trooper-o-matic.appspot.com/alerts').then(function(trooper_data)
{
These two requests should be done in parallel. You can use Promise.all to
execute code once they've both loaded.

https://codereview.chromium.org/476903004/diff/120001/Tools/GardeningServer/m...
Tools/GardeningServer/model/ct-failures.html:117: for (var key in data) {
Use Object.keys http://sugarjs.com/api/Object/keys. You can find other example
around the code that do this.

https://codereview.chromium.org/476903004/diff/120001/Tools/GardeningServer/m...
Tools/GardeningServer/model/ct-failures.html:120: trooper_failures.push(new
CTTrooperFailureGroup(data[key][tree], key, tree));
Don't store the raw data object we get from the server. Instead, use a model
class so that the things exposed are clearly documented and so there's a place
to attach related code. In this case, if you do the CTTrooperFailureGroupData
thing, then you could expose the fields you want on that.

https://codereview.chromium.org/476903004/diff/120001/Tools/GardeningServer/s...
File Tools/GardeningServer/sheriff-o-matic.html (right):

https://codereview.chromium.org/476903004/diff/120001/Tools/GardeningServer/s...
Tools/GardeningServer/sheriff-o-matic.html:25: <script type='text/javascript'
src='//www.google.com/jsapi'></script>
Nit: Leave out the type attribute. It doesn't do anything here.

https://codereview.chromium.org/476903004/diff/120001/Tools/GardeningServer/t...
File Tools/GardeningServer/test/run-unit-tests.html (right):

https://codereview.chromium.org/476903004/diff/120001/Tools/GardeningServer/t...
Tools/GardeningServer/test/run-unit-tests.html:19: <script
type='text/javascript' src='//www.google.com/jsapi'></script>
Ditto.

https://codereview.chromium.org/476903004/diff/120001/Tools/GardeningServer/u...
File Tools/GardeningServer/ui/ct-cq-latency-card.html (right):

https://codereview.chromium.org/476903004/diff/120001/Tools/GardeningServer/u...
Tools/GardeningServer/ui/ct-cq-latency-card.html:17: color: red;
No red. I'm OK with using color on this page to denote different things, but
everything on this page is an alert of some form, so everything on the page
could be red.

https://codereview.chromium.org/476903004/diff/120001/Tools/GardeningServer/u...
File Tools/GardeningServer/ui/ct-failure-card.html (left):

https://codereview.chromium.org/476903004/diff/120001/Tools/GardeningServer/u...
Tools/GardeningServer/ui/ct-failure-card.html:64: .snoozed {
Let's put the snoozed out here and put it on a div wrapping the cards. That way
we can avoid duplicating the snooze styling and class in each card.

https://codereview.chromium.org/476903004/diff/120001/Tools/GardeningServer/u...
File Tools/GardeningServer/ui/ct-failure-card.html (right):

https://codereview.chromium.org/476903004/diff/120001/Tools/GardeningServer/u...
Tools/GardeningServer/ui/ct-failure-card.html:60: <template if="{{ group.type ==
'sheriff' }}">
Instead of ct-failure-card containing different types of cards, I was picturing
that the different card would be a sibling to ct-failure-card and then each card
type would include the buttons via composition. That way, the failure card isn't
littered with stuff that doesn't make sense for it (e.g. ct-cq-latency-card
shouldn't know anything about commitLists).

https://codereview.chromium.org/476903004/diff/120001/Tools/GardeningServer/u...
File Tools/GardeningServer/ui/ct-sheriff-card.html (right):

https://codereview.chromium.org/476903004/diff/120001/Tools/GardeningServer/u...
Tools/GardeningServer/ui/ct-sheriff-card.html:11: <polymer-element
name="ct-sheriff-card" attributes="group">
s/sheriff/bot-failure/ or something like that. We'll likely end up showing some
of the sheriff-o-matic.appspot.com/alerts failures on the trooper page
eventually.

https://codereview.chromium.org/476903004/diff/120001/Tools/GardeningServer/u...
Tools/GardeningServer/ui/ct-sheriff-card.html:37: <script>
Leave out the script here and put a noscript attribute on the polymer-element.

https://codereview.chromium.org/476903004/diff/120001/Tools/GardeningServer/u...
File Tools/GardeningServer/ui/ct-tree-status-card.html (right):

https://codereview.chromium.org/476903004/diff/120001/Tools/GardeningServer/u...
Tools/GardeningServer/ui/ct-tree-status-card.html:30: </div>
This formatting is funky.

https://codereview.chromium.org/476903004/diff/120001/Tools/GardeningServer/u...
Tools/GardeningServer/ui/ct-tree-status-card.html:33:
Polymer('ct-tree-status-card', {
No need for the string name here. It's inferred for imports.

https://codereview.chromium.org/476903004/diff/120001/Tools/GardeningServer/u...
File Tools/GardeningServer/ui/test/ct-failure-card-tests.html (right):

https://codereview.chromium.org/476903004/diff/120001/Tools/GardeningServer/u...
Tools/GardeningServer/ui/test/ct-failure-card-tests.html:38: 'missing sheriff
card') != null);
This explanatory string is in the wrong place.

[eseidel, 2014-09-02 15:17:16]

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

[eseidel, 2014-09-02 15:17:17]

I'm not sure which opinion you're looking for.  My opinions on feed structure
are mostly on code structure.  We currently pull from one feed, but I expect
we'll eventually have many more than one.  I'd rather build separate feeds fit
to their purpose than hack everything into builder_alerts.  We can always stand
up an aggregator/cacher for sheriff-o-matic in front of our N feeds.

[shans, 2014-09-04 01:59:43]

> 1. This page should just show alerts (i.e. things that are > exceeding the
alert threshold). When there are no alerts, > it should show partytime.

This already happens.

> 2. I don't think we want the graphs. The text is enough
> IMO. Particularly, this page will soon have dozens of 
> alerts, so we don't want to devote this much screen real
> estate to something that's sufficiently covered by the
> text.

I added the graphs because the text wasn't making it easy to see what was going
on. Rather than nuke the graphs, I've shrunk them so that the size of the cards
are constrained by the buttons on the right.

> 3. Once we get rid of the graphs, we can simplify the 
> card structure to just have ct-bot-failure-card and 
> ct-text-failure-card or something like that.

Card structure simplified.

> 4. Instead of adding a "link" link, lets have the examine > page iframe the
appropriate trooper-o-matic.appspot page.

Done.

Sounds like the feeds can stay as they are for now.

BTW, I'll be OOO until the end of September from Friday (Sydney time) and will
have very limited availability for working on this, unfortunately.

https://codereview.chromium.org/476903004/diff/120001/Tools/GardeningServer/m...
File Tools/GardeningServer/model/ct-failure-group.html (right):

https://codereview.chromium.org/476903004/diff/120001/Tools/GardeningServer/m...
Tools/GardeningServer/model/ct-failure-group.html:20: function
CTTrooperFailureGroup(data, type, tree, annotation) {
On 2014/09/02 02:35:11, ojan-only-code-yellow-reviews wrote:
> Rather than using inheritance for this, in other places I've used create
> methods. CTFailureGroup.createSheriffGroup and
> CTFailureGroup.createTrooperGroup. Actually, even better here would be to just
> have one sub-class and use composition for the different behaviors. So,
> something like...
> 
> function CTFailureGroup(data, type, annotation) {...}
> 
> Where data is one of CTTrooperFailureGroupData or CTSheriffFailureGroupData,
> which has all the appropriate data and methods (e.g. now CTFailureGroup
wouldn't
> know anything about commitLists).

Done.

https://codereview.chromium.org/476903004/diff/120001/Tools/GardeningServer/m...
File Tools/GardeningServer/model/ct-failures.html (right):

https://codereview.chromium.org/476903004/diff/120001/Tools/GardeningServer/m...
Tools/GardeningServer/model/ct-failures.html:17: this.failures = {}
On 2014/09/02 02:35:11, ojan-only-code-yellow-reviews wrote:
> This won't update right until https://codereview.chromium.org/526633002/ lands
> since you don't clear the old alerts. Might want to rebase your patch on top
of
> that one.

Acknowledged.

https://codereview.chromium.org/476903004/diff/120001/Tools/GardeningServer/m...
Tools/GardeningServer/model/ct-failures.html:82:
net.json('http://trooper-o-matic.appspot.com/alerts').then(function(trooper_data)
{
On 2014/09/02 02:35:11, ojan-only-code-yellow-reviews wrote:
> These two requests should be done in parallel. You can use Promise.all to
> execute code once they've both loaded.

Done.

https://codereview.chromium.org/476903004/diff/120001/Tools/GardeningServer/m...
Tools/GardeningServer/model/ct-failures.html:117: for (var key in data) {
On 2014/09/02 02:35:11, ojan-only-code-yellow-reviews wrote:
> Use Object.keys http://sugarjs.com/api/Object/keys. You can find other example
> around the code that do this.

Done.

https://codereview.chromium.org/476903004/diff/120001/Tools/GardeningServer/m...
Tools/GardeningServer/model/ct-failures.html:120: trooper_failures.push(new
CTTrooperFailureGroup(data[key][tree], key, tree));
On 2014/09/02 02:35:11, ojan-only-code-yellow-reviews wrote:
> Don't store the raw data object we get from the server. Instead, use a model
> class so that the things exposed are clearly documented and so there's a place
> to attach related code. In this case, if you do the CTTrooperFailureGroupData
> thing, then you could expose the fields you want on that.

Done.

https://codereview.chromium.org/476903004/diff/120001/Tools/GardeningServer/s...
File Tools/GardeningServer/sheriff-o-matic.html (right):

https://codereview.chromium.org/476903004/diff/120001/Tools/GardeningServer/s...
Tools/GardeningServer/sheriff-o-matic.html:25: <script type='text/javascript'
src='//www.google.com/jsapi'></script>
On 2014/09/02 02:35:11, ojan-only-code-yellow-reviews wrote:
> Nit: Leave out the type attribute. It doesn't do anything here.

Done.

https://codereview.chromium.org/476903004/diff/120001/Tools/GardeningServer/t...
File Tools/GardeningServer/test/run-unit-tests.html (right):

https://codereview.chromium.org/476903004/diff/120001/Tools/GardeningServer/t...
Tools/GardeningServer/test/run-unit-tests.html:19: <script
type='text/javascript' src='//www.google.com/jsapi'></script>
On 2014/09/02 02:35:11, ojan-only-code-yellow-reviews wrote:
> Ditto.

Done.

https://codereview.chromium.org/476903004/diff/120001/Tools/GardeningServer/u...
File Tools/GardeningServer/ui/ct-cq-latency-card.html (right):

https://codereview.chromium.org/476903004/diff/120001/Tools/GardeningServer/u...
Tools/GardeningServer/ui/ct-cq-latency-card.html:17: color: red;
On 2014/09/02 02:35:11, ojan-only-code-yellow-reviews wrote:
> No red. I'm OK with using color on this page to denote different things, but
> everything on this page is an alert of some form, so everything on the page
> could be red.

Done.

https://codereview.chromium.org/476903004/diff/120001/Tools/GardeningServer/u...
File Tools/GardeningServer/ui/ct-failure-card.html (left):

https://codereview.chromium.org/476903004/diff/120001/Tools/GardeningServer/u...
Tools/GardeningServer/ui/ct-failure-card.html:64: .snoozed {
On 2014/09/02 02:35:11, ojan-only-code-yellow-reviews wrote:
> Let's put the snoozed out here and put it on a div wrapping the cards. That
way
> we can avoid duplicating the snooze styling and class in each card.

Done.

https://codereview.chromium.org/476903004/diff/120001/Tools/GardeningServer/u...
File Tools/GardeningServer/ui/ct-failure-card.html (right):

https://codereview.chromium.org/476903004/diff/120001/Tools/GardeningServer/u...
Tools/GardeningServer/ui/ct-failure-card.html:60: <template if="{{ group.type ==
'sheriff' }}">
On 2014/09/02 02:35:11, ojan-only-code-yellow-reviews wrote:
> Instead of ct-failure-card containing different types of cards, I was
picturing
> that the different card would be a sibling to ct-failure-card and then each
card
> type would include the buttons via composition. That way, the failure card
isn't
> littered with stuff that doesn't make sense for it (e.g. ct-cq-latency-card
> shouldn't know anything about commitLists).

I think the spirit of this is done now (ct-trooper-card doesn't know anything
about commitLists, neither sub-card type knows about annotations as that's
handled at this level).

https://codereview.chromium.org/476903004/diff/120001/Tools/GardeningServer/u...
File Tools/GardeningServer/ui/ct-sheriff-card.html (right):

https://codereview.chromium.org/476903004/diff/120001/Tools/GardeningServer/u...
Tools/GardeningServer/ui/ct-sheriff-card.html:11: <polymer-element
name="ct-sheriff-card" attributes="group">
On 2014/09/02 02:35:11, ojan-only-code-yellow-reviews wrote:
> s/sheriff/bot-failure/ or something like that. We'll likely end up showing
some
> of the sheriff-o-matic.appspot.com/alerts failures on the trooper page
> eventually.

Done.

https://codereview.chromium.org/476903004/diff/120001/Tools/GardeningServer/u...
Tools/GardeningServer/ui/ct-sheriff-card.html:37: <script>
On 2014/09/02 02:35:11, ojan-only-code-yellow-reviews wrote:
> Leave out the script here and put a noscript attribute on the polymer-element.

Done.

https://codereview.chromium.org/476903004/diff/120001/Tools/GardeningServer/u...
File Tools/GardeningServer/ui/ct-tree-status-card.html (right):

https://codereview.chromium.org/476903004/diff/120001/Tools/GardeningServer/u...
Tools/GardeningServer/ui/ct-tree-status-card.html:30: </div>
On 2014/09/02 02:35:11, ojan-only-code-yellow-reviews wrote:
> This formatting is funky.

Acknowledged.

https://codereview.chromium.org/476903004/diff/120001/Tools/GardeningServer/u...
Tools/GardeningServer/ui/ct-tree-status-card.html:33:
Polymer('ct-tree-status-card', {
On 2014/09/02 02:35:11, ojan-only-code-yellow-reviews wrote:
> No need for the string name here. It's inferred for imports.

Done.

https://codereview.chromium.org/476903004/diff/120001/Tools/GardeningServer/u...
File Tools/GardeningServer/ui/test/ct-failure-card-tests.html (right):

https://codereview.chromium.org/476903004/diff/120001/Tools/GardeningServer/u...
Tools/GardeningServer/ui/test/ct-failure-card-tests.html:38: 'missing sheriff
card') != null);
On 2014/09/02 02:35:11, ojan-only-code-yellow-reviews wrote:
> This explanatory string is in the wrong place.

Done.

[ojan, 2014-09-04 02:58:44]

Bummer that you won't be around for a few weeks. Is there anyone else who can
take this over?

https://codereview.chromium.org/476903004/diff/140001/Tools/GardeningServer/m...
File Tools/GardeningServer/model/ct-failures.html (right):

https://codereview.chromium.org/476903004/diff/140001/Tools/GardeningServer/m...
Tools/GardeningServer/model/ct-failures.html:124: if (failure.should_alert)
Put curlies.

https://codereview.chromium.org/476903004/diff/140001/Tools/GardeningServer/s...
File Tools/GardeningServer/sheriff-o-matic.html (right):

https://codereview.chromium.org/476903004/diff/140001/Tools/GardeningServer/s...
Tools/GardeningServer/sheriff-o-matic.html:25: <script
src='//www.google.com/jsapi'></script>
I'd rather we not include script tags at all. We use html imports everywhere so
that the component that actually draws the chart imports it and we can use
include-what-you-use style importing.

Also, if we're going to include a charting library, I'd rather we include
something more modern and maintained like http://www.chartjs.org/. But, I don't
want to block you landing this patch on that...so, if you don't want to do that,
can you make a ct-chart component and use that? Then the ct-chart component can
have a FIXME to move to chartjs and nothing outside the component would know
about this library. I just don't want us building a whole set of other charts
and stuff on this library, so hopefully a FIXME will be enough to dissuade
people.

https://codereview.chromium.org/476903004/diff/140001/Tools/GardeningServer/u...
File Tools/GardeningServer/ui/ct-failure-card.html (right):

https://codereview.chromium.org/476903004/diff/140001/Tools/GardeningServer/u...
Tools/GardeningServer/ui/ct-failure-card.html:63: <div class="{{ { snoozed:
group.isSnoozed, card: true } | tokenList }}">
"card {{ { snoozed: ... } | tokenList }}"

https://codereview.chromium.org/476903004/diff/140001/Tools/GardeningServer/u...
Tools/GardeningServer/ui/ct-failure-card.html:67: <template if="{{
group.data.type != 'sheriff' }}">
Nit: how about == 'trooper' instead?

https://codereview.chromium.org/476903004/diff/140001/Tools/GardeningServer/u...
Tools/GardeningServer/ui/ct-failure-card.html:97: commitLog: null,
In this design we're still passing a commitLog where the trooper one doesn't
need to know that and we're passing a tree that is unused by the trooper card as
well.

https://codereview.chromium.org/476903004/diff/140001/Tools/GardeningServer/u...
Tools/GardeningServer/ui/ct-failure-card.html:118: if (this.group &&
this.group.commitList && this.commitLog)
Isn't this still the card that doesn't have commit lists knowing about commit
lists?

https://codereview.chromium.org/476903004/diff/140001/Tools/GardeningServer/u...
File Tools/GardeningServer/ui/ct-results-panel.html (right):

https://codereview.chromium.org/476903004/diff/140001/Tools/GardeningServer/u...
Tools/GardeningServer/ui/ct-results-panel.html:54: <template if="{{
!(failures.length || failures.url) }}">
Nit: I prefer inverting these sorts of bools instead of needing the parens:
!failures.length && !failures.url

https://codereview.chromium.org/476903004/diff/140001/Tools/GardeningServer/u...
Tools/GardeningServer/ui/ct-results-panel.html:58: <iframe src='{{ failures.url
}}'></iframe>
Indentation is off.

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

https://codereview.chromium.org/476903004/diff/140001/Tools/GardeningServer/u...
Tools/GardeningServer/ui/ct-sheriff-o-matic.html:115: var dataToExamine =
event.detail.dataToExamine();
I'd rather that this code do different things depending on the failure group
type (i.e. select different tabs). The <ct-results-panel> shares almost nothing
in the two cases. That way you can also pass the CTTrooperFailureGroupData or
CTSheriffFailureGroupData directly to the panel. No need to get a dataToExamine
method.

https://codereview.chromium.org/476903004/diff/140001/Tools/GardeningServer/u...
File Tools/GardeningServer/ui/ct-trooper-card.html (right):

https://codereview.chromium.org/476903004/diff/140001/Tools/GardeningServer/u...
Tools/GardeningServer/ui/ct-trooper-card.html:38: groupChanged: function() {
Are there any tests for this code? I'd at least like a test of each switch case
that shows we get to the end of this function. You could do that by checking
that a chart got inserted into the chart div.

https://codereview.chromium.org/476903004/diff/140001/Tools/GardeningServer/u...
Tools/GardeningServer/ui/ct-trooper-card.html:81: return;
Should we console.error here?

https://codereview.chromium.org/476903004/diff/140001/Tools/GardeningServer/u...
File Tools/GardeningServer/ui/test/ct-failure-stream-tests.html (right):

https://codereview.chromium.org/476903004/diff/140001/Tools/GardeningServer/u...
Tools/GardeningServer/ui/test/ct-failure-stream-tests.html:44: stream.groups =
[new CTFailureGroup(new CTSheriffFailureGroupData([], undefined)), snoozed];
No need to pass undefined for arguments you're leaving out if they're the last
argument of the function.

https://codereview.chromium.org/476903004/diff/140001/Tools/GardeningServer/u...
Tools/GardeningServer/ui/test/ct-failure-stream-tests.html:44: stream.groups =
[new CTFailureGroup(new CTSheriffFailureGroupData([], undefined)), snoozed];
I know the old code was doing this...but, no need to pass undefined for
arguments you're leaving out if they're the last argument of the function.

[blink-reviews, 2014-09-04 17:34:13]

Shane, Mike, is there anyone else in Sydney that Mike can brain dump the
state of this to and have them take over?  This is really important for the
CY and we cannot wait until the end of Sept to have it completed.


On Wed, Sep 3, 2014 at 7:58 PM, <ojan@chromium.org> wrote:

> Bummer that you won't be around for a few weeks. Is there anyone else who
> can
> take this over?
>
>
> https://codereview.chromium.org/476903004/diff/140001/
> Tools/GardeningServer/model/ct-failures.html
> File Tools/GardeningServer/model/ct-failures.html (right):
>
> https://codereview.chromium.org/476903004/diff/140001/
> Tools/GardeningServer/model/ct-failures.html#newcode124
> Tools/GardeningServer/model/ct-failures.html:124: if
> (failure.should_alert)
> Put curlies.
>
> https://codereview.chromium.org/476903004/diff/140001/
> Tools/GardeningServer/sheriff-o-matic.html
> File Tools/GardeningServer/sheriff-o-matic.html (right):
>
> https://codereview.chromium.org/476903004/diff/140001/
> Tools/GardeningServer/sheriff-o-matic.html#newcode25
> Tools/GardeningServer/sheriff-o-matic.html:25: <script
> src='//www.google.com/jsapi'></script>
> I'd rather we not include script tags at all. We use html imports
> everywhere so that the component that actually draws the chart imports
> it and we can use include-what-you-use style importing.
>
> Also, if we're going to include a charting library, I'd rather we
> include something more modern and maintained like
> http://www.chartjs.org/. But, I don't want to block you landing this
> patch on that...so, if you don't want to do that, can you make a
> ct-chart component and use that? Then the ct-chart component can have a
> FIXME to move to chartjs and nothing outside the component would know
> about this library. I just don't want us building a whole set of other
> charts and stuff on this library, so hopefully a FIXME will be enough to
> dissuade people.
>
> https://codereview.chromium.org/476903004/diff/140001/
> Tools/GardeningServer/ui/ct-failure-card.html
> File Tools/GardeningServer/ui/ct-failure-card.html (right):
>
> https://codereview.chromium.org/476903004/diff/140001/
> Tools/GardeningServer/ui/ct-failure-card.html#newcode63
> Tools/GardeningServer/ui/ct-failure-card.html:63: <div class="{{ {
> snoozed: group.isSnoozed, card: true } | tokenList }}">
> "card {{ { snoozed: ... } | tokenList }}"
>
> https://codereview.chromium.org/476903004/diff/140001/
> Tools/GardeningServer/ui/ct-failure-card.html#newcode67
> Tools/GardeningServer/ui/ct-failure-card.html:67: <template if="{{
> group.data.type != 'sheriff' }}">
> Nit: how about == 'trooper' instead?
>
> https://codereview.chromium.org/476903004/diff/140001/
> Tools/GardeningServer/ui/ct-failure-card.html#newcode97
> Tools/GardeningServer/ui/ct-failure-card.html:97: commitLog: null,
> In this design we're still passing a commitLog where the trooper one
> doesn't need to know that and we're passing a tree that is unused by the
> trooper card as well.
>
> https://codereview.chromium.org/476903004/diff/140001/
> Tools/GardeningServer/ui/ct-failure-card.html#newcode118
> Tools/GardeningServer/ui/ct-failure-card.html:118: if (this.group &&
> this.group.commitList && this.commitLog)
> Isn't this still the card that doesn't have commit lists knowing about
> commit lists?
>
> https://codereview.chromium.org/476903004/diff/140001/
> Tools/GardeningServer/ui/ct-results-panel.html
> File Tools/GardeningServer/ui/ct-results-panel.html (right):
>
> https://codereview.chromium.org/476903004/diff/140001/
> Tools/GardeningServer/ui/ct-results-panel.html#newcode54
> Tools/GardeningServer/ui/ct-results-panel.html:54: <template if="{{
> !(failures.length || failures.url) }}">
> Nit: I prefer inverting these sorts of bools instead of needing the
> parens: !failures.length && !failures.url
>
> https://codereview.chromium.org/476903004/diff/140001/
> Tools/GardeningServer/ui/ct-results-panel.html#newcode58
> Tools/GardeningServer/ui/ct-results-panel.html:58: <iframe src='{{
> failures.url }}'></iframe>
> Indentation is off.
>
> https://codereview.chromium.org/476903004/diff/140001/
> Tools/GardeningServer/ui/ct-sheriff-o-matic.html
> File Tools/GardeningServer/ui/ct-sheriff-o-matic.html (right):
>
> https://codereview.chromium.org/476903004/diff/140001/
> Tools/GardeningServer/ui/ct-sheriff-o-matic.html#newcode115
> Tools/GardeningServer/ui/ct-sheriff-o-matic.html:115: var dataToExamine
> = event.detail.dataToExamine();
> I'd rather that this code do different things depending on the failure
> group type (i.e. select different tabs). The <ct-results-panel> shares
> almost nothing in the two cases. That way you can also pass the
> CTTrooperFailureGroupData or CTSheriffFailureGroupData directly to the
> panel. No need to get a dataToExamine method.
>
> https://codereview.chromium.org/476903004/diff/140001/
> Tools/GardeningServer/ui/ct-trooper-card.html
> File Tools/GardeningServer/ui/ct-trooper-card.html (right):
>
> https://codereview.chromium.org/476903004/diff/140001/
> Tools/GardeningServer/ui/ct-trooper-card.html#newcode38
> Tools/GardeningServer/ui/ct-trooper-card.html:38: groupChanged:
> function() {
> Are there any tests for this code? I'd at least like a test of each
> switch case that shows we get to the end of this function. You could do
> that by checking that a chart got inserted into the chart div.
>
> https://codereview.chromium.org/476903004/diff/140001/
> Tools/GardeningServer/ui/ct-trooper-card.html#newcode81
> Tools/GardeningServer/ui/ct-trooper-card.html:81: return;
> Should we console.error here?
>
> https://codereview.chromium.org/476903004/diff/140001/
> Tools/GardeningServer/ui/test/ct-failure-stream-tests.html
> File Tools/GardeningServer/ui/test/ct-failure-stream-tests.html (right):
>
> https://codereview.chromium.org/476903004/diff/140001/
> Tools/GardeningServer/ui/test/ct-failure-stream-tests.html#newcode44
> Tools/GardeningServer/ui/test/ct-failure-stream-tests.html:44:
> stream.groups = [new CTFailureGroup(new CTSheriffFailureGroupData([],
> undefined)), snoozed];
> No need to pass undefined for arguments you're leaving out if they're
> the last argument of the function.
>
> https://codereview.chromium.org/476903004/diff/140001/
> Tools/GardeningServer/ui/test/ct-failure-stream-tests.html#newcode44
> Tools/GardeningServer/ui/test/ct-failure-stream-tests.html:44:
> stream.groups = [new CTFailureGroup(new CTSheriffFailureGroupData([],
> undefined)), snoozed];
> I know the old code was doing this...but, no need to pass undefined for
> arguments you're leaving out if they're the last argument of the
> function.
>
> https://codereview.chromium.org/476903004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[shans, 2014-09-05 00:08:21]

RE: taking this over. Do you think this is far from landing? I'm somewhat
surprised the review process has been this detailed given the apparent urgency.

I can get through another round of reviews today and could probably land the
patch after that if everything is good. Otherwise I guess I'll try to do it in
fits and starts over the next week or two, or you can find someone to hand it
off to. Most of the Sydney team are OOO at the moment, but you could talk to
them on your Monday.

https://codereview.chromium.org/476903004/diff/140001/Tools/GardeningServer/m...
File Tools/GardeningServer/model/ct-failures.html (right):

https://codereview.chromium.org/476903004/diff/140001/Tools/GardeningServer/m...
Tools/GardeningServer/model/ct-failures.html:124: if (failure.should_alert)
On 2014/09/04 02:58:43, ojan-only-code-yellow-reviews wrote:
> Put curlies.

Done.

https://codereview.chromium.org/476903004/diff/140001/Tools/GardeningServer/s...
File Tools/GardeningServer/sheriff-o-matic.html (right):

https://codereview.chromium.org/476903004/diff/140001/Tools/GardeningServer/s...
Tools/GardeningServer/sheriff-o-matic.html:25: <script
src='//www.google.com/jsapi'></script>
On 2014/09/04 02:58:43, ojan-only-code-yellow-reviews wrote:
> I'd rather we not include script tags at all. We use html imports everywhere
so
> that the component that actually draws the chart imports it and we can use
> include-what-you-use style importing.

I actually tried that initially, but it seems that the google loading mechanism
doesn't play very nicely with HTML Imports and Polymer elements.

> Also, if we're going to include a charting library, I'd rather we include
> something more modern and maintained like http://www.chartjs.org/. But, I
don't
> want to block you landing this patch on that...so, if you don't want to do
that,
> can you make a ct-chart component and use that? Then the ct-chart component
can
> have a FIXME to move to chartjs and nothing outside the component would know
> about this library. I just don't want us building a whole set of other charts
> and stuff on this library, so hopefully a FIXME will be enough to dissuade
> people.

OK, switched to chartjsnew (which is chartjs enhanced with some of the extra
features that google charts provides).

https://codereview.chromium.org/476903004/diff/140001/Tools/GardeningServer/u...
File Tools/GardeningServer/ui/ct-failure-card.html (right):

https://codereview.chromium.org/476903004/diff/140001/Tools/GardeningServer/u...
Tools/GardeningServer/ui/ct-failure-card.html:63: <div class="{{ { snoozed:
group.isSnoozed, card: true } | tokenList }}">
On 2014/09/04 02:58:43, ojan-only-code-yellow-reviews wrote:
> "card {{ { snoozed: ... } | tokenList }}"

Done.

https://codereview.chromium.org/476903004/diff/140001/Tools/GardeningServer/u...
Tools/GardeningServer/ui/ct-failure-card.html:67: <template if="{{
group.data.type != 'sheriff' }}">
On 2014/09/04 02:58:43, ojan-only-code-yellow-reviews wrote:
> Nit: how about == 'trooper' instead?

I'd need to add a sub-type field. type currently contains 'cq_latency',
'tree_status', or 'build_times'.

https://codereview.chromium.org/476903004/diff/140001/Tools/GardeningServer/u...
Tools/GardeningServer/ui/ct-failure-card.html:97: commitLog: null,
On 2014/09/04 02:58:43, ojan-only-code-yellow-reviews wrote:
> In this design we're still passing a commitLog where the trooper one doesn't
> need to know that and we're passing a tree that is unused by the trooper card
as
> well.

On the other hand, if we made trooper-card a sibling to this, then we'd need to
duplicate snooze/unsnooze/linkBug/saveBug/removeBug functionality across both
cards.

https://codereview.chromium.org/476903004/diff/140001/Tools/GardeningServer/u...
Tools/GardeningServer/ui/ct-failure-card.html:118: if (this.group &&
this.group.commitList && this.commitLog)
On 2014/09/04 02:58:43, ojan-only-code-yellow-reviews wrote:
> Isn't this still the card that doesn't have commit lists knowing about commit
> lists?

No, this is the container of both card types deciding whether the card knows
about commit lists.

https://codereview.chromium.org/476903004/diff/140001/Tools/GardeningServer/u...
File Tools/GardeningServer/ui/ct-results-panel.html (right):

https://codereview.chromium.org/476903004/diff/140001/Tools/GardeningServer/u...
Tools/GardeningServer/ui/ct-results-panel.html:54: <template if="{{
!(failures.length || failures.url) }}">
On 2014/09/04 02:58:43, ojan-only-code-yellow-reviews wrote:
> Nit: I prefer inverting these sorts of bools instead of needing the parens:
> !failures.length && !failures.url

Done.

https://codereview.chromium.org/476903004/diff/140001/Tools/GardeningServer/u...
Tools/GardeningServer/ui/ct-results-panel.html:58: <iframe src='{{ failures.url
}}'></iframe>
On 2014/09/04 02:58:43, ojan-only-code-yellow-reviews wrote:
> Indentation is off.

Done.

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

https://codereview.chromium.org/476903004/diff/140001/Tools/GardeningServer/u...
Tools/GardeningServer/ui/ct-sheriff-o-matic.html:115: var dataToExamine =
event.detail.dataToExamine();
On 2014/09/04 02:58:43, ojan-only-code-yellow-reviews wrote:
> I'd rather that this code do different things depending on the failure group
> type (i.e. select different tabs). The <ct-results-panel> shares almost
nothing
> in the two cases. That way you can also pass the CTTrooperFailureGroupData or
> CTSheriffFailureGroupData directly to the panel. No need to get a
dataToExamine
> method.

I'm not sure how this would work with the history tracking stuff.

https://codereview.chromium.org/476903004/diff/140001/Tools/GardeningServer/u...
File Tools/GardeningServer/ui/ct-trooper-card.html (right):

https://codereview.chromium.org/476903004/diff/140001/Tools/GardeningServer/u...
Tools/GardeningServer/ui/ct-trooper-card.html:38: groupChanged: function() {
On 2014/09/04 02:58:44, ojan-only-code-yellow-reviews wrote:
> Are there any tests for this code? I'd at least like a test of each switch
case
> that shows we get to the end of this function. You could do that by checking
> that a chart got inserted into the chart div.

Done.

https://codereview.chromium.org/476903004/diff/140001/Tools/GardeningServer/u...
Tools/GardeningServer/ui/ct-trooper-card.html:81: return;
On 2014/09/04 02:58:44, ojan-only-code-yellow-reviews wrote:
> Should we console.error here?

Done.

https://codereview.chromium.org/476903004/diff/140001/Tools/GardeningServer/u...
File Tools/GardeningServer/ui/test/ct-failure-stream-tests.html (right):

https://codereview.chromium.org/476903004/diff/140001/Tools/GardeningServer/u...
Tools/GardeningServer/ui/test/ct-failure-stream-tests.html:44: stream.groups =
[new CTFailureGroup(new CTSheriffFailureGroupData([], undefined)), snoozed];
On 2014/09/04 02:58:44, ojan-only-code-yellow-reviews wrote:
> No need to pass undefined for arguments you're leaving out if they're the last
> argument of the function.

Acknowledged.

https://codereview.chromium.org/476903004/diff/140001/Tools/GardeningServer/u...
Tools/GardeningServer/ui/test/ct-failure-stream-tests.html:44: stream.groups =
[new CTFailureGroup(new CTSheriffFailureGroupData([], undefined)), snoozed];
On 2014/09/04 02:58:44, ojan-only-code-yellow-reviews wrote:
> I know the old code was doing this...but, no need to pass undefined for
> arguments you're leaving out if they're the last argument of the function.

Done.

[ojan, 2014-09-05 00:24:47]

lgtm. Please fix the nits before committing.

Thanks for going through the back and forth here. While we have a sense of
urgency, we want to build for the long-term first and foremost.

https://codereview.chromium.org/476903004/diff/160001/Tools/GardeningServer/u...
File Tools/GardeningServer/ui/ct-chart.html (right):

https://codereview.chromium.org/476903004/diff/160001/Tools/GardeningServer/u...
Tools/GardeningServer/ui/ct-chart.html:11: <style>
Don't need this style block in here, right?

https://codereview.chromium.org/476903004/diff/160001/Tools/GardeningServer/u...
Tools/GardeningServer/ui/ct-chart.html:27: optionsChanged: function() {
Use observe for this.

http://www.polymer-project.org/docs/polymer/polymer.html#observeblock

https://codereview.chromium.org/476903004/diff/160001/Tools/GardeningServer/u...
Tools/GardeningServer/ui/ct-chart.html:27: optionsChanged: function() {
Use observe for this.

http://www.polymer-project.org/docs/polymer/polymer.html#observeblock

https://codereview.chromium.org/476903004/diff/160001/Tools/GardeningServer/u...
File Tools/GardeningServer/ui/ct-trooper-card.html (right):

https://codereview.chromium.org/476903004/diff/160001/Tools/GardeningServer/u...
Tools/GardeningServer/ui/ct-trooper-card.html:30: <ct-chart table={{table}}
options={{options}} width=400 height=100></ct-chart>
Put spaces inside the curlies.

https://codereview.chromium.org/476903004/diff/160001/Tools/GardeningServer/u...
File Tools/GardeningServer/ui/test/ct-trooper-card-tests.html (right):

https://codereview.chromium.org/476903004/diff/160001/Tools/GardeningServer/u...
Tools/GardeningServer/ui/test/ct-trooper-card-tests.html:33: delete
__googleVisualizationAbstractRendererElementsCount__;
Here and below, you don't need this anymore now that you're not using the google
visualization library, right?

[shans, 2014-09-05 01:44:00]

All done.

Note that this will require a bower update before pushing to the server.

https://codereview.chromium.org/476903004/diff/160001/Tools/GardeningServer/u...
File Tools/GardeningServer/ui/ct-chart.html (right):

https://codereview.chromium.org/476903004/diff/160001/Tools/GardeningServer/u...
Tools/GardeningServer/ui/ct-chart.html:11: <style>
On 2014/09/05 00:24:47, ojan-only-code-yellow-reviews wrote:
> Don't need this style block in here, right?

Done.

https://codereview.chromium.org/476903004/diff/160001/Tools/GardeningServer/u...
Tools/GardeningServer/ui/ct-chart.html:27: optionsChanged: function() {
On 2014/09/05 00:24:47, ojan-only-code-yellow-reviews wrote:
> Use observe for this.
> 
> http://www.polymer-project.org/docs/polymer/polymer.html#observeblock

Done.

https://codereview.chromium.org/476903004/diff/160001/Tools/GardeningServer/u...
Tools/GardeningServer/ui/ct-chart.html:27: optionsChanged: function() {
On 2014/09/05 00:24:47, ojan-only-code-yellow-reviews wrote:
> Use observe for this.
> 
> http://www.polymer-project.org/docs/polymer/polymer.html#observeblock

Done.

https://codereview.chromium.org/476903004/diff/160001/Tools/GardeningServer/u...
File Tools/GardeningServer/ui/ct-trooper-card.html (right):

https://codereview.chromium.org/476903004/diff/160001/Tools/GardeningServer/u...
Tools/GardeningServer/ui/ct-trooper-card.html:30: <ct-chart table={{table}}
options={{options}} width=400 height=100></ct-chart>
On 2014/09/05 00:24:47, ojan-only-code-yellow-reviews wrote:
> Put spaces inside the curlies.

Done.

https://codereview.chromium.org/476903004/diff/160001/Tools/GardeningServer/u...
File Tools/GardeningServer/ui/test/ct-trooper-card-tests.html (right):

https://codereview.chromium.org/476903004/diff/160001/Tools/GardeningServer/u...
Tools/GardeningServer/ui/test/ct-trooper-card-tests.html:33: delete
__googleVisualizationAbstractRendererElementsCount__;
On 2014/09/05 00:24:47, ojan-only-code-yellow-reviews wrote:
> Here and below, you don't need this anymore now that you're not using the
google
> visualization library, right?

Done.

[shans, 2014-09-05 01:44:12]

The CQ bit was checked by shans@chromium.org

[commit-bot: I haz the power, 2014-09-05 01:44:22]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/shans@chromium.org/476903004/180001

[commit-bot: I haz the power, 2014-09-05 03:40:51]

Committed patchset #10 (id:180001) as 181431