Move commit list and builder latest revisions up to ct-unexpected-failures

Move commit list and builder latest revisions up to ct-unexpected-failures

Create a model directory for model classes. We should never be using
dumb objects as our model (even in tests!). Having explicit classes makes
more clear the types of the objects a component takes in. Also,
it gives a clear location for putting logic related to that bit of the
model.

Moving the commit list and the builder revisions up is a step in the
direction of enabling https://codereview.chromium.org/402603007. 

NOTRY=true

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

[ojan, 2014-07-19 02:02:58]

esprehn and I chatted on Wednesday about data models, core-ajax, testing, etc.
He convinced me that we should have dedicated model classes instead of dumb
objects and that we should push down data from the top (particularly XHR'ed
data) instead of side-loading it. This patch is a first step in that direction
and is the vast majority of the work needed for
https://codereview.chromium.org/402603007.

I've come to believe that the polymer style core-ajax or ct-commit-data are an
anti-pattern. There might still be a good use-case for the MonoState pattern,
but I haven't encountered it yet.

Sorry it's so big. It's converting a big chunk of the model to classes and
adding tests for a lot of previously untested code. Future ones will be smaller.

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

https://codereview.chromium.org/404463003/diff/1/Tools/GardeningServer/model/...
Tools/GardeningServer/model/ct-commit.html:8: function CTCommit(author, message)
{
This is a hugely simplified from svn-log.js. I didn't see the point in
supporting a bunch of effectively dead code.

[abarth-chromium, 2014-07-19 02:40:04]

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

https://codereview.chromium.org/404463003/diff/1/Tools/GardeningServer/model/...
Tools/GardeningServer/model/ct-commit-log-mock.html:9: <link rel='import'
href='ct-commit-mock.html'>
can we drop the ct prefix?

[ojan, 2014-07-19 02:44:22]

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

https://codereview.chromium.org/404463003/diff/1/Tools/GardeningServer/model/...
Tools/GardeningServer/model/ct-commit-log-mock.html:9: <link rel='import'
href='ct-commit-mock.html'>
On 2014/07/19 02:40:04, abarth wrote:
> can we drop the ct prefix?

I thought about that, but the model is intimately tied to the components that
use it. So, if we were to move the component that depends on this model object,
we'd move the model object along with it. So, you still want namespacing, right?

I thought about using a different namespace, but concluded that it being in a
different directory was sufficient.

Certainly open to suggestions/criticisms/alternatives. This patch is by no means
intended to be the final result. Now is the right time to discuss high-level
design and/or style things related to this change, since there will be similar
followup patches once this lands (e.g. converting ct-failure-analyzer to a model
class).

[abarth-chromium, 2014-07-21 06:36:27]

LGTM

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

https://codereview.chromium.org/404463003/diff/1/Tools/GardeningServer/model/...
Tools/GardeningServer/model/ct-commit-log.html:28:
CTCommitLog.prototype._processXml = function(xml) {
_processXml -> _processXML ?

https://codereview.chromium.org/404463003/diff/1/Tools/GardeningServer/model/...
Tools/GardeningServer/model/ct-commit-log.html:31: var message =
logentry.getElementsByTagName('content')[0].textContent;
Why trim the author but not the message?

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

https://codereview.chromium.org/404463003/diff/1/Tools/GardeningServer/model/...
Tools/GardeningServer/model/ct-commit.html:8: function CTCommit(author, message)
{
That's fine.

https://codereview.chromium.org/404463003/diff/1/Tools/GardeningServer/model/...
Tools/GardeningServer/model/ct-commit.html:10: this.message = message.trim();
I see, we do the trimming in different layers...  Seems like we should be
consistent.

https://codereview.chromium.org/404463003/diff/1/Tools/GardeningServer/model/...
Tools/GardeningServer/model/ct-commit.html:16: var regexp = /git-svn-id:
svn:\/\/svn.chromium.org\/blink\/trunk@(\d+)/;
This isn't very general...

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

https://codereview.chromium.org/404463003/diff/1/Tools/GardeningServer/ui/ct-...
Tools/GardeningServer/ui/ct-commit-list.html:40: this._revisions.push(i);
There's no better way in sugar to do this?  Looks like there is:

http://sugarjs.com/ranges

https://codereview.chromium.org/404463003/diff/1/Tools/GardeningServer/ui/ct-...
File Tools/GardeningServer/ui/ct-commit.html (right):

https://codereview.chromium.org/404463003/diff/1/Tools/GardeningServer/ui/ct-...
Tools/GardeningServer/ui/ct-commit.html:41: return trac.changesetURL(revision);
CTCommit could know it's URL.

[michaelpg, 2014-07-21 07:28:48]

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

https://codereview.chromium.org/404463003/diff/1/Tools/GardeningServer/model/...
Tools/GardeningServer/model/ct-commit-log-mock.html:9: <link rel='import'
href='ct-commit-mock.html'>
On 2014/07/19 02:44:22, ojan-only-code-yellow-reviews wrote:
> On 2014/07/19 02:40:04, abarth wrote:
> > can we drop the ct prefix?
> 
> I thought about that, but the model is intimately tied to the components that
> use it. So, if we were to move the component that depends on this model
object,
> we'd move the model object along with it. So, you still want namespacing,
right?
> 
> I thought about using a different namespace, but concluded that it being in a
> different directory was sufficient.
> 
> Certainly open to suggestions/criticisms/alternatives. This patch is by no
means
> intended to be the final result. Now is the right time to discuss high-level
> design and/or style things related to this change, since there will be similar
> followup patches once this lands (e.g. converting ct-failure-analyzer to a
model
> class).

I wouldn't mind something like
  <ct-model-commit>
just so it's clear in other components' templates which components are data and
which are UI.

Come to think of it, what does "ct" stand for?

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

https://codereview.chromium.org/404463003/diff/1/Tools/GardeningServer/model/...
Tools/GardeningServer/model/ct-commit-log.html:29:
Array.prototype.forEach.call(xml.getElementsByTagName('entry'),
function(logentry) {
nit: can we keep to an 80-char line length?

Array.prototype.forEach.call(xml.getElementsByTagName('entry'),
                                                      function(logentry) {
  ...

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

https://codereview.chromium.org/404463003/diff/1/Tools/GardeningServer/model/...
Tools/GardeningServer/model/ct-commit.html:11: this.revision =
this._findRevision();
this._findRevision(this.message)?

https://codereview.chromium.org/404463003/diff/1/Tools/GardeningServer/model/...
Tools/GardeningServer/model/ct-commit.html:12: this.summary =
this.message.split('\n')[0];
Are these messages always reasonable sizes or might there be a ton of lines?

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

https://codereview.chromium.org/404463003/diff/1/Tools/GardeningServer/ui/ct-...
Tools/GardeningServer/ui/ct-builder-grid.html:156: return Object.keys(builders);
Can you simplify this? Something like:

return Object.keys(this.builderLatestRevisions).filter(function(builder) {
  return parseInt(this.builderLatestRevisions[builder].blink, 10)
      < revision;
});

https://codereview.chromium.org/404463003/diff/1/Tools/GardeningServer/ui/ct-...
Tools/GardeningServer/ui/ct-builder-grid.html:163: builders.push.apply(builders,
this._buildersInFlightForRevision(revision + 1));
again, can we keep this to 80 chars? Or is that not something we want to do for
this project?

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

https://codereview.chromium.org/404463003/diff/1/Tools/GardeningServer/ui/ct-...
Tools/GardeningServer/ui/ct-unexpected-failures.html:33: <ct-failure-stream
groups="{{ failureGroups }}" builderLatestRevisions="{{ builderLatestRevisions
}}" commits="{{revisionLog.commits}}"></ct-failure-stream>
{{ revisionLog.commits }}

[ojan, 2014-07-21 19:58:05]

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

https://codereview.chromium.org/404463003/diff/1/Tools/GardeningServer/model/...
Tools/GardeningServer/model/ct-commit-log-mock.html:9: <link rel='import'
href='ct-commit-mock.html'>
ct == chromium-tools. In a future world in which we share code between tools,
we'll move some of these to a shared base library. Really we should be
distinguishing between ct and sm (sheriff-o-matic) already, but we can make that
change when we actually start putting the shared code in a shared place.

As to the naming, I don't feel strongly. There is no DOM in the model directory.
So, there's:
CTCommit == model
<ct-commit> == ui

We could rename the file to include model in the filename, but that seemed
redundant, e.g. model/ct-model-commit.html, model/ct-model-commit-log.html, etc.

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

https://codereview.chromium.org/404463003/diff/1/Tools/GardeningServer/model/...
Tools/GardeningServer/model/ct-commit-log.html:28:
CTCommitLog.prototype._processXml = function(xml) {
On 2014/07/21 06:36:26, abarth wrote:
> _processXml -> _processXML ?

I really dislike that style. That's how to get abominations like XMLHttpRequest.
XMLHTTPRequest.

https://codereview.chromium.org/404463003/diff/1/Tools/GardeningServer/model/...
Tools/GardeningServer/model/ct-commit-log.html:29:
Array.prototype.forEach.call(xml.getElementsByTagName('entry'),
function(logentry) {
On 2014/07/21 07:28:48, michaelpg wrote:
> nit: can we keep to an 80-char line length?
> 
> Array.prototype.forEach.call(xml.getElementsByTagName('entry'),
>                                                       function(logentry) {
>   ...

I'm not a huge fan of our hybrid blink/chromium style we're using for this
rewrite (blinks lack of a line-length limit and chromiums 2-space indent). I
blame abarth. That said, the rest of the code doesn't do 80 chars, so I'm not
inclined to do so.

https://codereview.chromium.org/404463003/diff/1/Tools/GardeningServer/model/...
Tools/GardeningServer/model/ct-commit-log.html:31: var message =
logentry.getElementsByTagName('content')[0].textContent;
On 2014/07/21 06:36:26, abarth wrote:
> Why trim the author but not the message?

This was just copy-paste from the old code. I didn't even notice the trimming of
author. What's funny is that message is trimmed in CTCommit. Will fix to just
trim up here.

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

https://codereview.chromium.org/404463003/diff/1/Tools/GardeningServer/model/...
Tools/GardeningServer/model/ct-commit.html:10: this.message = message.trim();
Fixed.

https://codereview.chromium.org/404463003/diff/1/Tools/GardeningServer/model/...
Tools/GardeningServer/model/ct-commit.html:11: this.revision =
this._findRevision();
Whoops. I forgot to remove the message argument to findRevision.

https://codereview.chromium.org/404463003/diff/1/Tools/GardeningServer/model/...
Tools/GardeningServer/model/ct-commit.html:12: this.summary =
this.message.split('\n')[0];
They're whatever size the commit description is. This is a silly way to grab the
first line. I just landed on this because I started with complicated svn-log.js
code and didn't notice I ended up with something silly. I put a FIXME to use
indexOf/substring for now since I don't want to create conflicts for
https://codereview.chromium.org/402603007.

https://codereview.chromium.org/404463003/diff/1/Tools/GardeningServer/model/...
Tools/GardeningServer/model/ct-commit.html:16: var regexp = /git-svn-id:
svn:\/\/svn.chromium.org\/blink\/trunk@(\d+)/;
Didn't even notice this. Copy-pasta from the old code. Added FIXME for now.

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

https://codereview.chromium.org/404463003/diff/1/Tools/GardeningServer/ui/ct-...
Tools/GardeningServer/ui/ct-builder-grid.html:156: return Object.keys(builders);
Much better. Although, I got rid of the hard to read line wrapping. :)

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

https://codereview.chromium.org/404463003/diff/1/Tools/GardeningServer/ui/ct-...
Tools/GardeningServer/ui/ct-commit-list.html:40: this._revisions.push(i);
Heh. I felt silly writing this code. I'm not sure how to make ranges work with
MDV since they only expose an "every" method that takes a callback for each
entry. Suggestions welcome.

https://codereview.chromium.org/404463003/diff/1/Tools/GardeningServer/ui/ct-...
File Tools/GardeningServer/ui/ct-commit.html (right):

https://codereview.chromium.org/404463003/diff/1/Tools/GardeningServer/ui/ct-...
Tools/GardeningServer/ui/ct-commit.html:41: return trac.changesetURL(revision);
Yup. I fix this in https://codereview.chromium.org/402603007/.

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

https://codereview.chromium.org/404463003/diff/1/Tools/GardeningServer/ui/ct-...
Tools/GardeningServer/ui/ct-unexpected-failures.html:33: <ct-failure-stream
groups="{{ failureGroups }}" builderLatestRevisions="{{ builderLatestRevisions
}}" commits="{{revisionLog.commits}}"></ct-failure-stream>
Done.

[ojan, 2014-07-21 19:58:54]

The CQ bit was checked by ojan@chromium.org

[commit-bot: I haz the power, 2014-07-21 19:59:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ojan@chromium.org/404463003/20001

[commit-bot: I haz the power, 2014-07-21 20:02:20]

Change committed as 178582

[esprehn, 2014-07-21 20:11:07]

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

https://codereview.chromium.org/404463003/diff/1/Tools/GardeningServer/ui/ct-...
Tools/GardeningServer/ui/ct-commit-list.html:40: this._revisions.push(i);
On 2014/07/21 19:58:04, ojan-only-code-yellow-reviews wrote:
> Heh. I felt silly writing this code. I'm not sure how to make ranges work with
> MDV since they only expose an "every" method that takes a callback for each
> entry. Suggestions welcome. 

every returns an array, ex Number.range(2, 8).every(2)

[ojan, 2014-07-21 20:14:02]

On 2014/07/21 at 20:11:07, esprehn wrote:
>
https://codereview.chromium.org/404463003/diff/1/Tools/GardeningServer/ui/ct-...
> File Tools/GardeningServer/ui/ct-commit-list.html (right):
> 
>
https://codereview.chromium.org/404463003/diff/1/Tools/GardeningServer/ui/ct-...
> Tools/GardeningServer/ui/ct-commit-list.html:40: this._revisions.push(i);
> On 2014/07/21 19:58:04, ojan-only-code-yellow-reviews wrote:
> > Heh. I felt silly writing this code. I'm not sure how to make ranges work
with
> > MDV since they only expose an "every" method that takes a callback for each
> > entry. Suggestions welcome. 
> 
> every returns an array, ex Number.range(2, 8).every(2)

Ah. I missed that in the documentation. Will fix.

[michaelpg, 2014-07-22 05:47:37]

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

https://codereview.chromium.org/404463003/diff/1/Tools/GardeningServer/model/...
Tools/GardeningServer/model/ct-commit-log-mock.html:9: <link rel='import'
href='ct-commit-mock.html'>
On 2014/07/21 19:58:04, ojan-only-code-yellow-reviews wrote:
> ct == chromium-tools. In a future world in which we share code between tools,
> we'll move some of these to a shared base library. Really we should be
> distinguishing between ct and sm (sheriff-o-matic) already, but we can make
that
> change when we actually start putting the shared code in a shared place.
> 
> As to the naming, I don't feel strongly. There is no DOM in the model
directory.
> So, there's:
> CTCommit == model
> <ct-commit> == ui
> 
> We could rename the file to include model in the filename, but that seemed
> redundant, e.g. model/ct-model-commit.html, model/ct-model-commit-log.html,
etc.

Oops, I didn't realize that there actually was a ct-commit UI element as opposed
to it just holding data. In that case, this is great.