Add the revision details widget to sheriff-o-matic

Add the revision details widget to sheriff-o-matic

Reproduce the functionality of the old revision details information
from garden-o-matic in sheriff-o-matic.

R=ojan@chromium.org
NOTRY=true

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

[ojan, 2014-07-21 23:22:38]

https://codereview.chromium.org/410483002/diff/1/Tools/GardeningServer/script...
File Tools/GardeningServer/scripts/model.js (right):

https://codereview.chromium.org/410483002/diff/1/Tools/GardeningServer/script...
Tools/GardeningServer/scripts/model.js:119:
model.latestRevisionWithNoBuildersInFlight = function()
This all needs to be updated for the new code. I suggest making
builderLatestRevisions into a BuilderRevisions model class and putting this
logic on there.

https://codereview.chromium.org/410483002/diff/1/Tools/GardeningServer/ui/ct-...
File Tools/GardeningServer/ui/ct-revision-details-tests.html (right):

https://codereview.chromium.org/410483002/diff/1/Tools/GardeningServer/ui/ct-...
Tools/GardeningServer/ui/ct-revision-details-tests.html:34:
equal(revisionDetails.shadowRoot.querySelector('#fullyProcessedRevision').innerText,
'1');
revisionDetails.$.fullyProcessedRevision.innerText

https://codereview.chromium.org/410483002/diff/1/Tools/GardeningServer/ui/ct-...
Tools/GardeningServer/ui/ct-revision-details-tests.html:35:
equal(revisionDetails.shadowRoot.querySelector('#trunkRevision').innerText,
'6');
Ditto

https://codereview.chromium.org/410483002/diff/1/Tools/GardeningServer/ui/ct-...
File Tools/GardeningServer/ui/ct-revision-details.html (right):

https://codereview.chromium.org/410483002/diff/1/Tools/GardeningServer/ui/ct-...
Tools/GardeningServer/ui/ct-revision-details.html:14: display: inline;
inline is the default. Do you actually want this to be inline?

https://codereview.chromium.org/410483002/diff/1/Tools/GardeningServer/ui/ct-...
Tools/GardeningServer/ui/ct-revision-details.html:16: .menu {
I've just been using tag names as selectors. No need for the class in a small
component like this.

https://codereview.chromium.org/410483002/diff/1/Tools/GardeningServer/ui/ct-...
Tools/GardeningServer/ui/ct-revision-details.html:20: .menuItem {
Can't just make the paper-item display flex and get rid of the wrapper div?

https://codereview.chromium.org/410483002/diff/1/Tools/GardeningServer/ui/ct-...
Tools/GardeningServer/ui/ct-revision-details.html:22: width: 100%;
Is this needed?

https://codereview.chromium.org/410483002/diff/1/Tools/GardeningServer/ui/ct-...
Tools/GardeningServer/ui/ct-revision-details.html:23: justify-content:
space-between;
space-between!

https://codereview.chromium.org/410483002/diff/1/Tools/GardeningServer/ui/ct-...
Tools/GardeningServer/ui/ct-revision-details.html:58: var revision = new Object;
s/new Object/{}

Or, even better:
return {
  number: revisionNumber,
  url: trac.changesetURL(revisionNumber),
};

Although, I think all this code goes away when you sync anyways since the
CTCommit has this info as properties.

https://codereview.chromium.org/410483002/diff/1/Tools/GardeningServer/ui/ct-...
Tools/GardeningServer/ui/ct-revision-details.html:77: this.trunkRevision =
this._createRevision(model.latestRevision());
_trunkRevision?

[leviw_travelin_and_unemployed, 2014-08-04 21:53:27]

PTAL

https://codereview.chromium.org/410483002/diff/1/Tools/GardeningServer/ui/ct-...
File Tools/GardeningServer/ui/ct-revision-details-tests.html (right):

https://codereview.chromium.org/410483002/diff/1/Tools/GardeningServer/ui/ct-...
Tools/GardeningServer/ui/ct-revision-details-tests.html:34:
equal(revisionDetails.shadowRoot.querySelector('#fullyProcessedRevision').innerText,
'1');
On 2014/07/21 23:22:37, ojan-only-code-yellow-reviews wrote:
> revisionDetails.$.fullyProcessedRevision.innerText

This doesn't actually work, and I'm not sure why that is. You'll notice that
every other test file also uses querySelector, not $.

[ojan, 2014-08-05 02:07:24]

A bunch of small things...

https://codereview.chromium.org/410483002/diff/20001/Tools/GardeningServer/mo...
File Tools/GardeningServer/model/ct-builder-revisions.html (right):

https://codereview.chromium.org/410483002/diff/20001/Tools/GardeningServer/mo...
Tools/GardeningServer/model/ct-builder-revisions.html:15: this[builder] =
CTCommit.createIncomplete('http://src.chromium.org/viewvc/blink?rev={revision}',
parseInt(builderData[builder]['blink']), 'blink');
-Always pass the 10 to parseInt, otherwise it can do crazy.
-Maybe wrap this line somehwere? Or move the parseInt'ed value to a local
variable?
-The sugarjs'ed version of Object.keys passes as a second argument the value in
the object, so you can use that instead of builderData[builder].

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

https://codereview.chromium.org/410483002/diff/20001/Tools/GardeningServer/mo...
Tools/GardeningServer/model/ct-commit-log.html:46: this.lastRevision[repository]
= parseInt(sortedCommits[sortedCommits.length - 1]);
sugarjs has first() and last() for these.

https://codereview.chromium.org/410483002/diff/20001/Tools/GardeningServer/mo...
Tools/GardeningServer/model/ct-commit-log.html:46: this.lastRevision[repository]
= parseInt(sortedCommits[sortedCommits.length - 1]);
sugarjs has first() and last() for these.

https://codereview.chromium.org/410483002/diff/20001/Tools/GardeningServer/ui...
File Tools/GardeningServer/ui/ct-popup-menu.html (right):

https://codereview.chromium.org/410483002/diff/20001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-popup-menu.html:14: overflow: hidden;
Why overflow:hidden?

https://codereview.chromium.org/410483002/diff/20001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-popup-menu.html:18: overflow-y: scroll;
s/scroll/auto?

https://codereview.chromium.org/410483002/diff/20001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-popup-menu.html:37: top: 0.5em;
Is this trying to center it? The better way to center is to use a flexbox and
align-items: center (or align-self: center just on the core-icon).

https://codereview.chromium.org/410483002/diff/20001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-popup-menu.html:40: visibility: hidden;
Don't think you need this if you have the opacity:0.

https://codereview.chromium.org/410483002/diff/20001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-popup-menu.html:44: <core-icon src="{{src}}"
icon="{{icon}}" on-click="{{ showAction }}"></core-icon>
Need spaces around src/icon

https://codereview.chromium.org/410483002/diff/20001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-popup-menu.html:48: <div id="scrim" class="hidden"
on-click="{{ hideAction }}"></div>
This is just to capture clicks? If so, you should just add a *capturing* click
handler to the body on attach and remove the handler on detach.

Also, add a FIXME to close the menu when you hit escape.

https://codereview.chromium.org/410483002/diff/20001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-popup-menu.html:53: showAction: function() {
Nit: _showAction and _hideAction

https://codereview.chromium.org/410483002/diff/20001/Tools/GardeningServer/ui...
File Tools/GardeningServer/ui/ct-revision-details-tests.html (right):

https://codereview.chromium.org/410483002/diff/20001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-revision-details-tests.html:18:
equal(revisionDetails.shadowRoot.querySelector('#fullyProcessedRevision'),
null);
It's very mysterious to me that revisionDetails.$.fullyProcessedRevision doesn't
work. There's something I'm not understanding. :( Doesn't need to block landing
the patch though.

https://codereview.chromium.org/410483002/diff/20001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-revision-details-tests.html:29:
commitLog._findFirstAndLastRevisions('blink');
Should CTCommitLogMock be the one doing this? Seems to me like a commit log that
hasn't already called this is in an invalid state.

https://codereview.chromium.org/410483002/diff/20001/Tools/GardeningServer/ui...
File Tools/GardeningServer/ui/ct-revision-details.html (right):

https://codereview.chromium.org/410483002/diff/20001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-revision-details.html:22: ct-popup-menu > div {
Just give the div a class name instead of using a child selector.

https://codereview.chromium.org/410483002/diff/20001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-revision-details.html:58:
sortedBuilders.sort(function (a, b) { return
this.builderLatestRevisions[a].revision -
this.builderLatestRevisions[b].revision;}.bind(this));
Nit: a little line-wrapping here would be more readable IMO. A newline before
the return and after the ; is what I'd do. ANd maybe local variables for
aRevision/bRevision.

[leviw_travelin_and_unemployed, 2014-08-05 20:51:18]

https://codereview.chromium.org/410483002/diff/20001/Tools/GardeningServer/mo...
File Tools/GardeningServer/model/ct-builder-revisions.html (right):

https://codereview.chromium.org/410483002/diff/20001/Tools/GardeningServer/mo...
Tools/GardeningServer/model/ct-builder-revisions.html:15: this[builder] =
CTCommit.createIncomplete('http://src.chromium.org/viewvc/blink?rev={revision}',
parseInt(builderData[builder]['blink']), 'blink');
On 2014/08/05 02:07:23, ojan-only-code-yellow-reviews wrote:
> -Always pass the 10 to parseInt, otherwise it can do crazy.
What 10? Is there a value I'm not parseInt-ing here?

> -Maybe wrap this line somehwere? Or move the parseInt'ed value to a local
> variable?
Done.

> -The sugarjs'ed version of Object.keys passes as a second argument the value
in
> the object, so you can use that instead of builderData[builder].
Done.

https://codereview.chromium.org/410483002/diff/20001/Tools/GardeningServer/ui...
File Tools/GardeningServer/ui/ct-popup-menu.html (right):

https://codereview.chromium.org/410483002/diff/20001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-popup-menu.html:18: overflow-y: scroll;
On 2014/08/05 02:07:23, ojan-only-code-yellow-reviews wrote:
> s/scroll/auto?

With scroll, we don't wrap the contents. With auto, the longest bots wrap (seems
like a blink bug :-/ ).

https://codereview.chromium.org/410483002/diff/20001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-popup-menu.html:40: visibility: hidden;
It will still hit test otherwise and get in the way of the contents under it.

https://codereview.chromium.org/410483002/diff/20001/Tools/GardeningServer/ui...
File Tools/GardeningServer/ui/ct-revision-details-tests.html (right):

https://codereview.chromium.org/410483002/diff/20001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-revision-details-tests.html:18:
equal(revisionDetails.shadowRoot.querySelector('#fullyProcessedRevision'),
null);
On 2014/08/05 02:07:23, ojan-only-code-yellow-reviews wrote:
> It's very mysterious to me that revisionDetails.$.fullyProcessedRevision
doesn't
> work. There's something I'm not understanding. :( Doesn't need to block
landing
> the patch though.

Beats me, but you will notice there are no other test files that do it.

https://codereview.chromium.org/410483002/diff/20001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-revision-details-tests.html:29:
commitLog._findFirstAndLastRevisions('blink');
On 2014/08/05 02:07:23, ojan-only-code-yellow-reviews wrote:
> Should CTCommitLogMock be the one doing this? Seems to me like a commit log
that
> hasn't already called this is in an invalid state.

Doing it in CTCommitLogMock breaks ct-commit-list: basic. It relies on behavior
in CTCommitLog.prototype.range which is gated by firstRevision. I think it's a
kind of crappy test.

[ojan, 2014-08-06 22:58:46]

https://codereview.chromium.org/410483002/diff/20001/Tools/GardeningServer/mo...
File Tools/GardeningServer/model/ct-builder-revisions.html (right):

https://codereview.chromium.org/410483002/diff/20001/Tools/GardeningServer/mo...
Tools/GardeningServer/model/ct-builder-revisions.html:15: this[builder] =
CTCommit.createIncomplete('http://src.chromium.org/viewvc/blink?rev={revision}',
parseInt(builderData[builder]['blink']), 'blink');
On 2014/08/05 20:51:17, leviw wrote:
> On 2014/08/05 02:07:23, ojan-only-code-yellow-reviews wrote:
> > -Always pass the 10 to parseInt, otherwise it can do crazy.
> What 10? Is there a value I'm not parseInt-ing here?
> 
> > -Maybe wrap this line somehwere? Or move the parseInt'ed value to a local
> > variable?
> Done.
> 
> > -The sugarjs'ed version of Object.keys passes as a second argument the value
> in
> > the object, so you can use that instead of builderData[builder].
> Done.

parseInt(builderData[builder]['blink'], 10)

That way it parses as decimal instead of inferring the radix.

https://codereview.chromium.org/410483002/diff/20001/Tools/GardeningServer/ui...
File Tools/GardeningServer/ui/ct-popup-menu.html (right):

https://codereview.chromium.org/410483002/diff/20001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-popup-menu.html:18: overflow-y: scroll;
On 2014/08/05 20:51:17, leviw wrote:
> On 2014/08/05 02:07:23, ojan-only-code-yellow-reviews wrote:
> > s/scroll/auto?
> 
> With scroll, we don't wrap the contents. With auto, the longest bots wrap
(seems
> like a blink bug :-/ ).

Def sounds like a blink bug. At least add a FIXME?

https://codereview.chromium.org/410483002/diff/20001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-popup-menu.html:40: visibility: hidden;
On 2014/08/05 20:51:17, leviw wrote:
> It will still hit test otherwise and get in the way of the contents under it.

Ah. Mind adding an explanatory comment?

https://codereview.chromium.org/410483002/diff/40001/Tools/GardeningServer/ui...
File Tools/GardeningServer/ui/ct-popup-menu.html (right):

https://codereview.chromium.org/410483002/diff/40001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-popup-menu.html:16: position: absolute;
We don't have an official style on this, but I've started sorting CSS properties
lexicographically.

https://codereview.chromium.org/410483002/diff/40001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-popup-menu.html:43: detached: function() {
Nit: inconsistent newlining. all your other functions in this file have a
newline between the functions.

https://codereview.chromium.org/410483002/diff/40001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-popup-menu.html:56: if (event.path[i] ===
this.$.icon || event.path[i] === this.$.menu) {
Can this just be:
if (event.path[i] === this)

?

Or, even better,
if (!event.path[i].find(function(element) { return element == this; })) {
  event.preventDefault();
  this.$.menu...
}

[ojan, 2014-08-06 22:59:01]

lgtm

[leviw_travelin_and_unemployed, 2014-08-06 23:37:11]

https://codereview.chromium.org/410483002/diff/40001/Tools/GardeningServer/ui...
File Tools/GardeningServer/ui/ct-popup-menu.html (right):

https://codereview.chromium.org/410483002/diff/40001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-popup-menu.html:16: position: absolute;
On 2014/08/06 22:58:46, ojan-only-code-yellow-reviews wrote:
> We don't have an official style on this, but I've started sorting CSS
properties
> lexicographically.

Done.

https://codereview.chromium.org/410483002/diff/40001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-popup-menu.html:43: detached: function() {
On 2014/08/06 22:58:46, ojan-only-code-yellow-reviews wrote:
> Nit: inconsistent newlining. all your other functions in this file have a
> newline between the functions.

Done.

https://codereview.chromium.org/410483002/diff/40001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-popup-menu.html:56: if (event.path[i] ===
this.$.icon || event.path[i] === this.$.menu) {
On 2014/08/06 22:58:46, ojan-only-code-yellow-reviews wrote:
> Can this just be:
> if (event.path[i] === this)
> 
> ?
> 
> Or, even better,
> if (!event.path[i].find(function(element) { return element == this; })) {
>   event.preventDefault();
>   this.$.menu...
> }

It can be the former. NodeLists confusingly don't have a find method.

[ojan, 2014-08-06 23:38:40]

https://codereview.chromium.org/410483002/diff/40001/Tools/GardeningServer/ui...
File Tools/GardeningServer/ui/ct-popup-menu.html (right):

https://codereview.chromium.org/410483002/diff/40001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-popup-menu.html:31: <core-icon id="icon"
src="{{src}}" icon="{{icon}}" on-click="{{ _toggleAction }}"></core-icon>
Spaces in the curlies!

[leviw_travelin_and_unemployed, 2014-08-06 23:43:53]

On 2014/08/06 at 23:38:40, ojan wrote:
>
https://codereview.chromium.org/410483002/diff/40001/Tools/GardeningServer/ui...
> File Tools/GardeningServer/ui/ct-popup-menu.html (right):
> 
>
https://codereview.chromium.org/410483002/diff/40001/Tools/GardeningServer/ui...
> Tools/GardeningServer/ui/ct-popup-menu.html:31: <core-icon id="icon"
src="{{src}}" icon="{{icon}}" on-click="{{ _toggleAction }}"></core-icon>
> Spaces in the curlies!

Curlies spaced!

[leviw_travelin_and_unemployed, 2014-08-06 23:43:57]

The CQ bit was checked by leviw@chromium.org

[commit-bot: I haz the power, 2014-08-06 23:44:28]

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

[commit-bot: I haz the power, 2014-08-06 23:46:46]

Change committed as 179666