Move commit list out its own model.

Move commit list out its own model.

This CL creates a model to hold information on the commit lists. This is
in prepartion for adding the ability to expand the commit list.

BUG=399715
NOTRY=true

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

[dsinclair, 2014-08-12 17:57:46]

PTAL.

This needs tests (and I probably broke other tests), but I wanted to make sure I
was at least on the right road before handling testing.

[ojan, 2014-08-12 21:08:36]

l g t m once it has tests.

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

https://codereview.chromium.org/464963003/diff/1/Tools/GardeningServer/model/...
Tools/GardeningServer/model/ct-commit-list.html:26: if (commits.length == 0)
Nit: normally we just do "if (!commits.length)"

https://codereview.chromium.org/464963003/diff/1/Tools/GardeningServer/model/...
Tools/GardeningServer/model/ct-commit-list.html:54: /* The existence of commits
is a pre-condition to this method. */
Nit: that's pretty clear from a glance at the code IMO

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

https://codereview.chromium.org/464963003/diff/1/Tools/GardeningServer/ui/ct-...
Tools/GardeningServer/ui/ct-failure-stream.html:30: return new
CTCommitList(group, this.commits);
Not sure if it's better, but CTFailureGroup could have a commitlist method that
you call here. Then you could keep passing commits down to ct-failure-card and
ct-failure-card would pass group.commitList(this.commits) to ct-commit-list. The
marginal advantage I see to that is that ct-failure-stream doesn't need to know
about CTCommitLists at all, e.g. this would make testing easier.

Up to you.

[dsinclair, 2014-08-15 14:05:04]

PTAL.

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

https://codereview.chromium.org/464963003/diff/1/Tools/GardeningServer/model/...
Tools/GardeningServer/model/ct-commit-list.html:26: if (commits.length == 0)
On 2014/08/12 21:08:36, ojan-only-code-yellow-reviews wrote:
> Nit: normally we just do "if (!commits.length)"

Done.

https://codereview.chromium.org/464963003/diff/1/Tools/GardeningServer/model/...
Tools/GardeningServer/model/ct-commit-list.html:54: /* The existence of commits
is a pre-condition to this method. */
On 2014/08/12 21:08:36, ojan-only-code-yellow-reviews wrote:
> Nit: that's pretty clear from a glance at the code IMO

Done.

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

https://codereview.chromium.org/464963003/diff/1/Tools/GardeningServer/ui/ct-...
Tools/GardeningServer/ui/ct-failure-stream.html:30: return new
CTCommitList(group, this.commits);
On 2014/08/12 21:08:36, ojan-only-code-yellow-reviews wrote:
> Not sure if it's better, but CTFailureGroup could have a commitlist method
that
> you call here. Then you could keep passing commits down to ct-failure-card and
> ct-failure-card would pass group.commitList(this.commits) to ct-commit-list.
The
> marginal advantage I see to that is that ct-failure-stream doesn't need to
know
> about CTCommitLists at all, e.g. this would make testing easier.
> 
> Up to you.

Done.

[ojan, 2014-08-15 17:36:56]

lgtm

One nit/question. Thanks for rushing this through!

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

https://codereview.chromium.org/464963003/diff/120001/Tools/GardeningServer/m...
Tools/GardeningServer/model/ct-commit-list.html:20: if (!this.revisionLog ||
!this.lastPassing || !this.firstFailing)
Is it actually possible not to have a revisionLog? As in, don't all the callers
always pass one in?

[dsinclair, 2014-08-15 18:31:56]

On 2014/08/15 17:36:56, ojan-only-code-yellow-reviews wrote:
> lgtm
> 
> One nit/question. Thanks for rushing this through!
> 
>
https://codereview.chromium.org/464963003/diff/120001/Tools/GardeningServer/m...
> File Tools/GardeningServer/model/ct-commit-list.html (right):
> 
>
https://codereview.chromium.org/464963003/diff/120001/Tools/GardeningServer/m...
> Tools/GardeningServer/model/ct-commit-list.html:20: if (!this.revisionLog ||
> !this.lastPassing || !this.firstFailing)
> Is it actually possible not to have a revisionLog? As in, don't all the
callers
> always pass one in?

I'm not actually sure, the CTFailureGroup passes through its commits but I don't
know if that's always existing in tests and actual code. 

I'd rather leave this in for safety and we can investigate cleaning it up later
if it really isn't needed.

[dsinclair, 2014-08-15 18:32:05]

The CQ bit was checked by dsinclair@chromium.org

[commit-bot: I haz the power, 2014-08-15 18:32:19]

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

[commit-bot: I haz the power, 2014-08-15 18:32:43]

Committed patchset #7 (120001) as 180385