[Sheriff-o-matic] Remove race condition on the commit list.

[Sheriff-o-matic] Remove race condition on the commit list.

By always modifying the same list of commits, we ensure that data binding does The Right Thing and updates when data comes in.

As well, renamed "revisionLog" to "commitLog" everywhere, to better reflect the data type.

BUG=405327
NOTRY=true

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

[Mathieu, 2014-08-20 17:33:48]

Hi Ojan, 

Have a look? I'm aware that the ground (repo) is shifting, but for now it's
here.

Thanks!

[ojan, 2014-08-20 18:47:51]

lgtm

Yay! Thanks so much for doing this. This patch is great except for if you can
add one more test and remove the log spam. No need for another round of reviews
unless you want me to take another look.

Also, you can NOTRY=true this since the CQ doesn't run these tests. Feel free to
land. The code hasn't moved yet. :)

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

https://codereview.chromium.org/485253004/diff/1/Tools/GardeningServer/ui/ct-...
Tools/GardeningServer/ui/ct-failure-card.html:117: return
this.group.commitList(this.commitLog);
Can you add a test that would have broken in the old code?

Particularly, create a ct-failure-card with an unpopulated commitLog. setTimout.
Assert that it has commits without descriptions. Then populate the commitLog.
setTimeout. Assert that it has commits *with* descriptions.

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

https://codereview.chromium.org/485253004/diff/1/Tools/GardeningServer/ui/ct-...
Tools/GardeningServer/ui/ct-unexpected-failures.html:60:
window.console.log('created CTCommitLog');
Leftover debugging code?

https://codereview.chromium.org/485253004/diff/1/Tools/GardeningServer/ui/ct-...
Tools/GardeningServer/ui/ct-unexpected-failures.html:76:
window.console.log('unexpected-failures: update');
Ditto

[michaelpg, 2014-08-20 19:32:32]

Took a look just out of curiosity, left a couple drive-by comments.

https://codereview.chromium.org/485253004/diff/1/Tools/GardeningServer/model/...
File Tools/GardeningServer/model/ct-commit-log.html (right):

https://codereview.chromium.org/485253004/diff/1/Tools/GardeningServer/model/...
Tools/GardeningServer/model/ct-commit-log.html:39: var commit =
CTCommit.create(author, message, repositoryUrl, repository);
This is a weird pattern to me, you're creating a commit and then maybe not using
it. If you make CTCommit._findRevision public static you could just pass it the
message to find the revision here and see whether a commit already exists before
creating one.

https://codereview.chromium.org/485253004/diff/1/Tools/GardeningServer/model/...
File Tools/GardeningServer/model/ct-repositories.html (right):

https://codereview.chromium.org/485253004/diff/1/Tools/GardeningServer/model/...
Tools/GardeningServer/model/ct-repositories.html:34: return
Object.keys(this.repositories).sort();
if this doesn't change, could we make it a property of the object rather than a
function? or is CTRepositories not intended to be "const"?

[Mathieu, 2014-08-21 14:08:21]

Hello, 

Please have a look, Ojan especially. Improved the data binding in failure-card,
added tests and addressed comments around checking for existing CTCommits.

Thanks!

https://codereview.chromium.org/485253004/diff/1/Tools/GardeningServer/model/...
File Tools/GardeningServer/model/ct-commit-log.html (right):

https://codereview.chromium.org/485253004/diff/1/Tools/GardeningServer/model/...
Tools/GardeningServer/model/ct-commit-log.html:39: var commit =
CTCommit.create(author, message, repositoryUrl, repository);
On 2014/08/20 19:32:32, michaelpg wrote:
> This is a weird pattern to me, you're creating a commit and then maybe not
using
> it. If you make CTCommit._findRevision public static you could just pass it
the
> message to find the revision here and see whether a commit already exists
before
> creating one. 

Done.

https://codereview.chromium.org/485253004/diff/1/Tools/GardeningServer/model/...
File Tools/GardeningServer/model/ct-repositories.html (right):

https://codereview.chromium.org/485253004/diff/1/Tools/GardeningServer/model/...
Tools/GardeningServer/model/ct-repositories.html:34: return
Object.keys(this.repositories).sort();
On 2014/08/20 19:32:32, michaelpg wrote:
> if this doesn't change, could we make it a property of the object rather than
a
> function? or is CTRepositories not intended to be "const"?

Done.

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

https://codereview.chromium.org/485253004/diff/1/Tools/GardeningServer/ui/ct-...
Tools/GardeningServer/ui/ct-failure-card.html:117: return
this.group.commitList(this.commitLog);
On 2014/08/20 18:47:51, ojan-only-code-yellow-reviews wrote:
> Can you add a test that would have broken in the old code?
> 
> Particularly, create a ct-failure-card with an unpopulated commitLog.
setTimout.
> Assert that it has commits without descriptions. Then populate the commitLog.
> setTimeout. Assert that it has commits *with* descriptions.

Done.

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

https://codereview.chromium.org/485253004/diff/1/Tools/GardeningServer/ui/ct-...
Tools/GardeningServer/ui/ct-unexpected-failures.html:60:
window.console.log('created CTCommitLog');
On 2014/08/20 18:47:51, ojan-only-code-yellow-reviews wrote:
> Leftover debugging code?

Done.

https://codereview.chromium.org/485253004/diff/1/Tools/GardeningServer/ui/ct-...
Tools/GardeningServer/ui/ct-unexpected-failures.html:76:
window.console.log('unexpected-failures: update');
On 2014/08/20 18:47:51, ojan-only-code-yellow-reviews wrote:
> Ditto

Oops! Proof I'm human!

[ojan, 2014-08-21 17:51:29]

lgtm

FYI, you can NOTRY=true these patches since they don't run on the CQ.

Also, yay! This bug is terrible!

https://codereview.chromium.org/485253004/diff/60001/Tools/GardeningServer/mo...
File Tools/GardeningServer/model/ct-repositories.html (right):

https://codereview.chromium.org/485253004/diff/60001/Tools/GardeningServer/mo...
Tools/GardeningServer/model/ct-repositories.html:31: this._names =
Object.keys(this.repositories).sort();
Nit: no need for the getter. Lets just make the property this.names.

[Mathieu, 2014-08-21 18:00:37]

Thanks! Submitting

https://codereview.chromium.org/485253004/diff/60001/Tools/GardeningServer/mo...
File Tools/GardeningServer/model/ct-repositories.html (right):

https://codereview.chromium.org/485253004/diff/60001/Tools/GardeningServer/mo...
Tools/GardeningServer/model/ct-repositories.html:31: this._names =
Object.keys(this.repositories).sort();
On 2014/08/21 17:51:29, ojan-only-code-yellow-reviews wrote:
> Nit: no need for the getter. Lets just make the property this.names.

Done.

[Mathieu, 2014-08-21 18:00:42]

The CQ bit was checked by mathp@chromium.org

[commit-bot: I haz the power, 2014-08-21 18:01:31]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mathp@chromium.org/485253004/80001

[commit-bot: I haz the power, 2014-08-21 18:02:37]

Committed patchset #3 (80001) as 180728