Adds a snooze button to the sheriff-o-matic ui

Adds a snooze button to the sheriff-o-matic ui

The button is configured to snooze the item for 1 hour. The item is deprioritized
in the UI by changing the opacity. Subsequent patches could separate snoozed
items out into a separate collapsible stream.

This patch adds the concept of annotations on a group of failures, 
persisting them in local storage. Subsequent patches should persist
the annotations in a frontend.

NOTRY=true
BUG=399967
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=179897

[dstockwell, 2014-08-06 01:55:13]

Small patch. Wanted to make sure this is headed in the right direction and check
if there's anything else you would like to see.

[ojan, 2014-08-06 04:03:02]

Yay! Thanks for working on this.

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

https://codereview.chromium.org/448503003/diff/1/Tools/GardeningServer/model/...
Tools/GardeningServer/model/ct-failure-group.html:14: 
Just one line break

https://codereview.chromium.org/448503003/diff/1/Tools/GardeningServer/model/...
Tools/GardeningServer/model/ct-failure-group.html:15: CTFailureGroup.prototype =
{
For better or worse, we've been using the style where you set each method on the
prototype. I'd be totally open to reconsidering that, but I'd want all our apps
to change to be consistent.

https://codereview.chromium.org/448503003/diff/1/Tools/GardeningServer/model/...
Tools/GardeningServer/model/ct-failure-group.html:16: get isSnoozed() {
IMO getters make for confusing code. More importantly, I think polymer might get
confused by getters. It won't properly detect when the value changes, e.g. if
you data-bind isSnoozed and then modify snoozeTime, I'm not sure polymer will
update the bound isSnoozed values. Maybe I'm wrong, maybe polymer handles this
fine.

Instead of this, I'd just change the CTFailureGroup to not store the snoozeTime
and instead just store an isSnoozed value. That does mean you can't dynamically
check the isSnoozed value later, but I think that's OK since we update every 30
seconds.

https://codereview.chromium.org/448503003/diff/1/Tools/GardeningServer/model/...
Tools/GardeningServer/model/ct-failure-group.html:21: this.snoozeTime = time;
I think it makes sense to implement this incrementally, but we need to store
this somewhere in the short-term since we update every 30 seconds. How about we
store the snooze values as JSON in localStorage? We can use the same JSON format
that we want sheriff-o-matic to serve up. That way this code and
ct-failure-analyzer will just need to be changed to store/get the json off the
server instead of localStorage once we're able to store the data on the server.

https://codereview.chromium.org/448503003/diff/1/Tools/GardeningServer/model/...
Tools/GardeningServer/model/ct-failure-group.html:22:
Object.getNotifier(this).notify({
Why did you need to do this? This might be working around the issue I mentioned
above about getters.

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

https://codereview.chromium.org/448503003/diff/1/Tools/GardeningServer/ui/ct-...
Tools/GardeningServer/ui/ct-failure-stream.html:24: Show snoozed failures:
<paper-toggle-button checked="{{ showSnoozed }}"></paper-toggle-button>
We should only show this if there are any snoozed failures. And lets put the
snoozed failures below the non-snoozed failures since they're theoretically the
failures the sheriff doesn't need to be thinking about.

I also had another idea. I can't decide if it's a good one or not. It's related
to crbug.com/400397. Instead of collapsing snoozed failures we could give them a
dedicated ct-failure-stream at the bottom. The advantage of that is that we
don't need to do any collapsing behavior and it doesn't take up any precious UI
space when there are non-snoozed failures to fix. The disadvantage is that we'd
have to clump together the snoozed tree closers, reliable failures and flaky
failures in the snoozing failure stream.

I slightly lean toward the latter approach. If you're OK with implementing it,
I'd like to give that a try first and only come back to this collapsing thing if
we decide the other UI doesn't work. WDYT?

In terms of how this would be implemented, what I pictured is that
ct-failure-analyzer, instead of having one flat list of failures, would have a
list of failures for each ct-failure-stream.

[ojan, 2014-08-06 04:05:59]

Oh, and of course we'll need tests before we can land this. :)

[dstockwell, 2014-08-06 06:55:13]

PTAL, applied most of your comments, but didn't separate out snoozed items into
another stream yet.

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

https://codereview.chromium.org/448503003/diff/1/Tools/GardeningServer/model/...
Tools/GardeningServer/model/ct-failure-group.html:14: 
On 2014/08/06 04:03:01, ojan-only-code-yellow-reviews wrote:
> Just one line break

Done.

https://codereview.chromium.org/448503003/diff/1/Tools/GardeningServer/model/...
Tools/GardeningServer/model/ct-failure-group.html:15: CTFailureGroup.prototype =
{
On 2014/08/06 04:03:01, ojan-only-code-yellow-reviews wrote:
> For better or worse, we've been using the style where you set each method on
the
> prototype. I'd be totally open to reconsidering that, but I'd want all our
apps
> to change to be consistent.

Done.

https://codereview.chromium.org/448503003/diff/1/Tools/GardeningServer/model/...
Tools/GardeningServer/model/ct-failure-group.html:16: get isSnoozed() {
On 2014/08/06 04:03:01, ojan-only-code-yellow-reviews wrote:
> IMO getters make for confusing code. More importantly, I think polymer might
get
> confused by getters. It won't properly detect when the value changes, e.g. if
> you data-bind isSnoozed and then modify snoozeTime, I'm not sure polymer will
> update the bound isSnoozed values. Maybe I'm wrong, maybe polymer handles this
> fine.
> 
> Instead of this, I'd just change the CTFailureGroup to not store the
snoozeTime
> and instead just store an isSnoozed value. That does mean you can't
dynamically
> check the isSnoozed value later, but I think that's OK since we update every
30
> seconds.

Done.

https://codereview.chromium.org/448503003/diff/1/Tools/GardeningServer/model/...
Tools/GardeningServer/model/ct-failure-group.html:21: this.snoozeTime = time;
On 2014/08/06 04:03:01, ojan-only-code-yellow-reviews wrote:
> I think it makes sense to implement this incrementally, but we need to store
> this somewhere in the short-term since we update every 30 seconds. How about
we
> store the snooze values as JSON in localStorage? We can use the same JSON
format
> that we want sheriff-o-matic to serve up. That way this code and
> ct-failure-analyzer will just need to be changed to store/get the json off the
> server instead of localStorage once we're able to store the data on the
server.

Done.

https://codereview.chromium.org/448503003/diff/1/Tools/GardeningServer/model/...
Tools/GardeningServer/model/ct-failure-group.html:22:
Object.getNotifier(this).notify({
On 2014/08/06 04:03:01, ojan-only-code-yellow-reviews wrote:
> Why did you need to do this? This might be working around the issue I
mentioned
> above about getters.

Yes, this allows the binding to the getter to be reevaluated.

Removed in favor of just storing isSnoozed.

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

https://codereview.chromium.org/448503003/diff/1/Tools/GardeningServer/ui/ct-...
Tools/GardeningServer/ui/ct-failure-stream.html:24: Show snoozed failures:
<paper-toggle-button checked="{{ showSnoozed }}"></paper-toggle-button>
On 2014/08/06 04:03:01, ojan-only-code-yellow-reviews wrote:
> We should only show this if there are any snoozed failures. And lets put the
> snoozed failures below the non-snoozed failures since they're theoretically
the
> failures the sheriff doesn't need to be thinking about.
> 
> I also had another idea. I can't decide if it's a good one or not. It's
related
> to crbug.com/400397. Instead of collapsing snoozed failures we could give them
a
> dedicated ct-failure-stream at the bottom. The advantage of that is that we
> don't need to do any collapsing behavior and it doesn't take up any precious
UI
> space when there are non-snoozed failures to fix. The disadvantage is that
we'd
> have to clump together the snoozed tree closers, reliable failures and flaky
> failures in the snoozing failure stream.
> 
> I slightly lean toward the latter approach. If you're OK with implementing it,
> I'd like to give that a try first and only come back to this collapsing thing
if
> we decide the other UI doesn't work. WDYT?
> 
> In terms of how this would be implemented, what I pictured is that
> ct-failure-analyzer, instead of having one flat list of failures, would have a
> list of failures for each ct-failure-stream.

I've left them in the stream (but dimmed) for this patch, let's split them out
into a separate stream in another patch.

[ojan, 2014-08-07 00:09:58]

lgtm

https://codereview.chromium.org/448503003/diff/80001/Tools/GardeningServer/mo...
File Tools/GardeningServer/model/ct-failure-group.html (right):

https://codereview.chromium.org/448503003/diff/80001/Tools/GardeningServer/mo...
Tools/GardeningServer/model/ct-failure-group.html:36:
Object.keys(newAnnotation).forEach(function(key) {
Nit: The sugarjs Object.keys takes a callback function, so you can do:
Object.keys(newAnnotation, function(key, value) { ... });

https://codereview.chromium.org/448503003/diff/80001/Tools/GardeningServer/mo...
Tools/GardeningServer/model/ct-failure-group.html:38: if (typeof value ==
'undefined') {
Nit: I'd just do:
if (value === undefined)
  ...
else
  ...

Or, even just
if (value)
  annotation[key] = value;
else
  delete annotation[key];

All nits though.

https://codereview.chromium.org/448503003/diff/80001/Tools/GardeningServer/mo...
Tools/GardeningServer/model/ct-failure-group.html:45: if
(Object.keys(annotation).length == 0) {
sugarjs has Object.size. So this can just be
if (Object.size(annotation))
  annotations[this.key] = annotation;
else
  delete annotations[this.key];

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

https://codereview.chromium.org/448503003/diff/80001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-failure-analyzer.html:37:
net.json('http://auto-sheriff.appspot.com/data').then(function(data) {
This is good for now. FYI, I'm starting to think the end result we want is for
the snooze annotations to be built in to the auto-sheriff data directly (i.e.
auto-sheriff would read the snooze data out of sheriff-o-matic's backend and
integrate the data into it's feed), i.e. it would just have the snooze
annotation as part of the entry in the range_groups field.

[dstockwell, 2014-08-11 01:20:43]

https://codereview.chromium.org/448503003/diff/80001/Tools/GardeningServer/mo...
File Tools/GardeningServer/model/ct-failure-group.html (right):

https://codereview.chromium.org/448503003/diff/80001/Tools/GardeningServer/mo...
Tools/GardeningServer/model/ct-failure-group.html:36:
Object.keys(newAnnotation).forEach(function(key) {
On 2014/08/07 00:09:57, ojan-only-code-yellow-reviews wrote:
> Nit: The sugarjs Object.keys takes a callback function, so you can do:
> Object.keys(newAnnotation, function(key, value) { ... });

Done.

https://codereview.chromium.org/448503003/diff/80001/Tools/GardeningServer/mo...
Tools/GardeningServer/model/ct-failure-group.html:38: if (typeof value ==
'undefined') {
On 2014/08/07 00:09:58, ojan-only-code-yellow-reviews wrote:
> Nit: I'd just do:
> if (value === undefined)
>   ...
> else
>   ...
> 
> Or, even just
> if (value)
>   annotation[key] = value;
> else
>   delete annotation[key];
> 
> All nits though.

Done.

https://codereview.chromium.org/448503003/diff/80001/Tools/GardeningServer/mo...
Tools/GardeningServer/model/ct-failure-group.html:45: if
(Object.keys(annotation).length == 0) {
On 2014/08/07 00:09:57, ojan-only-code-yellow-reviews wrote:
> sugarjs has Object.size. So this can just be
> if (Object.size(annotation))
>   annotations[this.key] = annotation;
> else
>   delete annotations[this.key];

Done.

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

https://codereview.chromium.org/448503003/diff/80001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-failure-analyzer.html:37:
net.json('http://auto-sheriff.appspot.com/data').then(function(data) {
On 2014/08/07 00:09:58, ojan-only-code-yellow-reviews wrote:
> This is good for now. FYI, I'm starting to think the end result we want is for
> the snooze annotations to be built in to the auto-sheriff data directly (i.e.
> auto-sheriff would read the snooze data out of sheriff-o-matic's backend and
> integrate the data into it's feed), i.e. it would just have the snooze
> annotation as part of the entry in the range_groups field.

Why does sheriff-o-matic need its own backend? Can't we just store annotations
directly in auto-sheriff. I'm not seeing the difference between auto-sheriff and
sheriff-o-matic.

[dstockwell, 2014-08-11 01:28:29]

The CQ bit was checked by dstockwell@chromium.org

[commit-bot: I haz the power, 2014-08-11 01:29:52]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dstockwell@chromium.org/448503003/120001

[commit-bot: I haz the power, 2014-08-11 01:31:27]

Change committed as 179897

[ojan, 2014-08-11 01:44:44]

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

https://codereview.chromium.org/448503003/diff/80001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-failure-analyzer.html:37:
net.json('http://auto-sheriff.appspot.com/data').then(function(data) {
On 2014/08/11 01:20:42, dstockwell wrote:
> On 2014/08/07 00:09:58, ojan-only-code-yellow-reviews wrote:
> > This is good for now. FYI, I'm starting to think the end result we want is
for
> > the snooze annotations to be built in to the auto-sheriff data directly
(i.e.
> > auto-sheriff would read the snooze data out of sheriff-o-matic's backend and
> > integrate the data into it's feed), i.e. it would just have the snooze
> > annotation as part of the entry in the range_groups field.
> 
> Why does sheriff-o-matic need its own backend? Can't we just store annotations
> directly in auto-sheriff. I'm not seeing the difference between auto-sheriff
and
> sheriff-o-matic.

Very shortly, in the next day or two, we'll be getting rid of auto-sheriff and
sheriff-o-matic will be the data source. At this point, all auto-sheriff does is
host a json file. Confusingly, it doesn't even generate the json file.

[jochen (gone - plz use gerrit), 2014-08-11 11:25:13]

this CL broke both ct-failure-stream.base and ct-failure-card.examine tests