Allow filing a bug from the "Link Bug" dialog

Allow filing a bug from the "Link Bug" dialog

R=ojan@chromium.org
NOTRY=true
BUG=399734
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=182866

[ojan, 2014-08-26 03:14:17]

Needs tests. :)

https://codereview.chromium.org/502293003/diff/1/Tools/GardeningServer/ui/ct-...
File Tools/GardeningServer/ui/ct-builder-grid.html (right):

https://codereview.chromium.org/502293003/diff/1/Tools/GardeningServer/ui/ct-...
Tools/GardeningServer/ui/ct-builder-grid.html:24: builders: null,
This really wants to be a model class. Can you create a CTBuilderList model
class that takes a list of failures as it's arguments and does all the work
below to create an array of CTBuilder's? Then have ct-failure-card create that
model and pass it down into ct-builder-grid. Then ct-failure-card doesn't need
to reach into ct-builder-grid to get this data.

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

https://codereview.chromium.org/502293003/diff/1/Tools/GardeningServer/ui/ct-...
Tools/GardeningServer/ui/ct-failure-card.html:97: <div><a id="fileBugLink"
target="_new" on-click="{{ fileBugClicked }}">File bug</a></div>
What's _new do? I've always done _blank.

https://codereview.chromium.org/502293003/diff/1/Tools/GardeningServer/ui/ct-...
Tools/GardeningServer/ui/ct-failure-card.html:98: <div style="display: hidden"
id="bugReminder">
Instead of toggling display with script, just wrap each of these in a <template
if>? You still need the onclick to set the member variable I suppose. Not sure
if it's better or worse.

https://codereview.chromium.org/502293003/diff/1/Tools/GardeningServer/ui/ct-...
Tools/GardeningServer/ui/ct-failure-card.html:113:
'summary=Build%20failure&comment=Build%20is%20broken:%0a',
Lets leave the summary out of this. google code has a dumb bug where it will
clear this value when you click on it. It's super annoying.

https://codereview.chromium.org/502293003/diff/1/Tools/GardeningServer/ui/ct-...
Tools/GardeningServer/ui/ct-failure-card.html:138: var message = '';
Can you move the function that generates this url (and _fileBugUrl) into
CTFailureGroup? You can pass it the commitLog as an argument to that function
for now, although probably we should just have a setCommitLog function on
CTFailureGroup so that we don't need to pass it in in multiple places.

https://codereview.chromium.org/502293003/diff/1/Tools/GardeningServer/ui/ct-...
Tools/GardeningServer/ui/ct-failure-card.html:139: for (var i = 0; i <
this.group.failures.length; ++i) {
Here and below, use forEach?

[cbiesinger, 2014-09-11 06:24:35]

PTAL. Test added.

https://codereview.chromium.org/502293003/diff/1/Tools/GardeningServer/ui/ct-...
File Tools/GardeningServer/ui/ct-builder-grid.html (right):

https://codereview.chromium.org/502293003/diff/1/Tools/GardeningServer/ui/ct-...
Tools/GardeningServer/ui/ct-builder-grid.html:24: builders: null,
On 2014/08/26 03:14:17, ojan-only-code-yellow-reviews wrote:
> This really wants to be a model class. Can you create a CTBuilderList model
> class that takes a list of failures as it's arguments and does all the work
> below to create an array of CTBuilder's? Then have ct-failure-card create that
> model and pass it down into ct-builder-grid. Then ct-failure-card doesn't need
> to reach into ct-builder-grid to get this data.

Done in the other CLs you reviewed.

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

https://codereview.chromium.org/502293003/diff/1/Tools/GardeningServer/ui/ct-...
Tools/GardeningServer/ui/ct-failure-card.html:97: <div><a id="fileBugLink"
target="_new" on-click="{{ fileBugClicked }}">File bug</a></div>
On 2014/08/26 03:14:17, ojan-only-code-yellow-reviews wrote:
> What's _new do? I've always done _blank.

Sigh, I misremembered the name. Fixed.

https://codereview.chromium.org/502293003/diff/1/Tools/GardeningServer/ui/ct-...
Tools/GardeningServer/ui/ct-failure-card.html:98: <div style="display: hidden"
id="bugReminder">
On 2014/08/26 03:14:17, ojan-only-code-yellow-reviews wrote:
> Instead of toggling display with script, just wrap each of these in a
<template
> if>? You still need the onclick to set the member variable I suppose. Not sure
> if it's better or worse.

Done.

https://codereview.chromium.org/502293003/diff/1/Tools/GardeningServer/ui/ct-...
Tools/GardeningServer/ui/ct-failure-card.html:113:
'summary=Build%20failure&comment=Build%20is%20broken:%0a',
On 2014/08/26 03:14:17, ojan-only-code-yellow-reviews wrote:
> Lets leave the summary out of this. google code has a dumb bug where it will
> clear this value when you click on it. It's super annoying.

Done.

https://codereview.chromium.org/502293003/diff/1/Tools/GardeningServer/ui/ct-...
Tools/GardeningServer/ui/ct-failure-card.html:138: var message = '';
On 2014/08/26 03:14:17, ojan-only-code-yellow-reviews wrote:
> Can you move the function that generates this url (and _fileBugUrl) into
> CTFailureGroup? You can pass it the commitLog as an argument to that function
> for now, although probably we should just have a setCommitLog function on
> CTFailureGroup so that we don't need to pass it in in multiple places.

I thought about that, but I'm really not sure that it belongs in the model
class. Isn't this link generation more of a UI thing, especially figuring out
exactly what the message should be?

But if you really want me to move it, I can.

https://codereview.chromium.org/502293003/diff/1/Tools/GardeningServer/ui/ct-...
Tools/GardeningServer/ui/ct-failure-card.html:139: for (var i = 0; i <
this.group.failures.length; ++i) {
On 2014/08/26 03:14:17, ojan-only-code-yellow-reviews wrote:
> Here and below, use forEach?

Done.

[ojan, 2014-09-11 23:44:25]

lgtm

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

https://codereview.chromium.org/502293003/diff/1/Tools/GardeningServer/ui/ct-...
Tools/GardeningServer/ui/ct-failure-card.html:138: var message = '';
On 2014/09/11 06:24:34, cbiesinger wrote:
> On 2014/08/26 03:14:17, ojan-only-code-yellow-reviews wrote:
> > Can you move the function that generates this url (and _fileBugUrl) into
> > CTFailureGroup? You can pass it the commitLog as an argument to that
function
> > for now, although probably we should just have a setCommitLog function on
> > CTFailureGroup so that we don't need to pass it in in multiple places.
> 
> I thought about that, but I'm really not sure that it belongs in the model
> class. Isn't this link generation more of a UI thing, especially figuring out
> exactly what the message should be?
> 
> But if you really want me to move it, I can.

Maybe you don't need to move the url generation to a helper, but you probably
should move the message generation. As an example, we're going to add a revert
button for changes and we'll want to share the message generation code.

[cbiesinger, 2014-09-27 00:32:29]

OK, you convinced me, I moved it to the model. This should be ready for checkin
and passes the tests, but I can't currently test it manually due to
"XMLHttpRequest cannot load http://sheriff-o-matic.appspot.com/alerts. No
'Access-Control-Allow-Origin' header is present on the requested resource.
Origin 'http://cbiesinger-z620.nyc.corp.google.com:8000' is therefore not
allowed access.", so I don't want to check it in just yet.

[cbiesinger, 2014-09-29 21:18:06]

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

https://codereview.chromium.org/502293003/diff/1/Tools/GardeningServer/ui/ct-...
Tools/GardeningServer/ui/ct-failure-card.html:98: <div style="display: hidden"
id="bugReminder">
On 2014/08/26 03:14:17, ojan-only-code-yellow-reviews wrote:
> Instead of toggling display with script, just wrap each of these in a
<template
> if>? You still need the onclick to set the member variable I suppose. Not sure
> if it's better or worse.

FYI I did this but had to move the template inside the <a>. Otherwise, toggling
the member variable will make the <a> disappear before the link click can open
the window (at least, that's what I think is happening).

[cbiesinger, 2014-09-29 21:18:12]

The CQ bit was checked by cbiesinger@chromium.org

[commit-bot: I haz the power, 2014-09-29 21:18:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/502293003/100001

[commit-bot: I haz the power, 2014-09-29 21:19:18]

Committed patchset #6 (id:100001) as 182866