[GOM] Improve the display of the roll information

[GOM] Improve the display of the roll information

The current UI was racy and would output the last roll
and current roll in random order. This change fixes this
and puts the roll information to a new line, so that it
stands out from the bot information.

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

[ojan, 2014-06-04 19:46:24]

Please add tests. There probably is already a test for this? Load
run_unittests.html from disk (no need to load from a server) is how you run
them.

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

https://codereview.chromium.org/314913002/diff/1/Tools/GardeningServer/script...
Tools/GardeningServer/scripts/ui.js:358:
this.appendChild(document.createElement("br"));
Meh. I prefer it on one line to conserve vertical space. But I don't feel
strongly enough about that to not lgtm.

[Julien - ping for review, 2014-06-05 00:33:50]

> Please add tests. There probably is already a test for this? Load
run_unittests.html from disk (no need to load from a server) is how you run
them.

There was no test for that so I added one. I also found a better fix using
Promise.all(...).

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

https://codereview.chromium.org/314913002/diff/1/Tools/GardeningServer/script...
Tools/GardeningServer/scripts/ui.js:358:
this.appendChild(document.createElement("br"));
On 2014/06/04 19:46:25, ojan wrote:
> Meh. I prefer it on one line to conserve vertical space. But I don't feel
> strongly enough about that to not lgtm.

You kinda did :-)

When doing a design, it's important to put associated information close together
(rule of proximity) but also somehow separate it from non-related information
(rule of contrast). This <br> is meant to make roll-related information (last
rolled revision and current roll) stand aside from bot related information
(trunk revision and last revision processed by the bots).

As it stands we have a really long line that's mixing 2 types of information and
makes it hard to parse IMO.

[Julien - ping for review, 2014-06-05 00:45:29]

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

https://codereview.chromium.org/314913002/diff/1/Tools/GardeningServer/script...
Tools/GardeningServer/scripts/ui.js:358:
this.appendChild(document.createElement("br"));
On 2014/06/05 00:33:51, Julien Chaffraix - PST wrote:
> On 2014/06/04 19:46:25, ojan wrote:
> > Meh. I prefer it on one line to conserve vertical space. But I don't feel
> > strongly enough about that to not lgtm.
> 
> You kinda did :-)
> 
> When doing a design, it's important to put associated information close
together
> (rule of proximity) but also somehow separate it from non-related information
> (rule of contrast). This <br> is meant to make roll-related information (last
> rolled revision and current roll) stand aside from bot related information
> (trunk revision and last revision processed by the bots).
> 
> As it stands we have a really long line that's mixing 2 types of information
and
> makes it hard to parse IMO.

I am open to better suggestions to separate the 2 types of information but I
definitely think that bundling them together hurts the usability of GOM.

[ojan, 2014-06-05 00:54:48]

lgtm

https://codereview.chromium.org/314913002/diff/20001/Tools/GardeningServer/sc...
File Tools/GardeningServer/scripts/ui.js (right):

https://codereview.chromium.org/314913002/diff/20001/Tools/GardeningServer/sc...
Tools/GardeningServer/scripts/ui.js:424: Promise.all([
checkout.lastBlinkRollRevision(), rollbot.fetchCurrentRoll()
]).then(function(results) {
Nit: I don't know if we have an official style rule for this, but I think we
normally wouldn't put the spaces after/before the [ and ]. The other places we
do arrays we don't put the spaces.

https://codereview.chromium.org/314913002/diff/20001/Tools/GardeningServer/sc...
File Tools/GardeningServer/scripts/ui_unittests.js (right):

https://codereview.chromium.org/314913002/diff/20001/Tools/GardeningServer/sc...
Tools/GardeningServer/scripts/ui_unittests.js:228: {
"messages":[],"base_url":"svn://svn.chromium.org/chrome/trunk/src","subject":"Blink
roll 540:550","closed":false,"issue":1000}
Nit: We're a little less consitent about the spaces here, but most places don't
put the space after the { and do put spaces after the commas and colons.

https://codereview.chromium.org/314913002/diff/20001/Tools/GardeningServer/sc...
Tools/GardeningServer/scripts/ui_unittests.js:257:
equal(revisionDetails.innerHTML, 'Latest revision processed by every bot:
<details><summary><a
href="http://src.chromium.org/viewvc/blink?view=rev&amp;revision=554">554<span
id="revisionPopUp"><table><tr><td><a
href="http://build.chromium.org/p/chromium.webkit/waterfall?builder=Linux">Linux</a></td><td>554</td></tr></table></span></a></summary><table><tr><td><a
href="http://build.chromium.org/p/chromium.webkit/waterfall?builder=Linux">Linux</a></td><td>554</td></tr></table></details>,
trunk is at <a
href="http://src.chromium.org/viewvc/blink?view=rev&amp;revision=555">555</a><br>Last
roll is to <a
href="http://src.chromium.org/viewvc/blink?view=rev&amp;revision=540">540</a>,
current autoroll <a href="https://codereview.chromium.org/1000">540:550</a>');
When I do innerHTML based tests, I usually format the string so that it's human
readable, e.g.

'Latest revision processed by every bot: ' +
    '<details>' +
        '<summary>' +
            '<a
href="http://src.chromium.org/viewvc/blink?view=rev&amp;revision=554">554' +
...

Alternately, instead of using innerHTML, I do:
var container = document.createElement('div');
container.innerHTML = revisionDetails.innerHTML;

var expectedUrls = [...];
var expectedLinkText = [...];
var links = container.querySelectorAll('a');
// Loop through links and verify the URLs and link text all match
// Verify anything else about the DOM that seems like valuable testing to you.

The advantage of the latter is that it makes it a bit easier to maintain, e.g.
if you just add a space or a comma or something you don't need to update this
test.

[Julien - ping for review, 2014-06-05 01:51:52]

https://codereview.chromium.org/314913002/diff/20001/Tools/GardeningServer/sc...
File Tools/GardeningServer/scripts/ui.js (right):

https://codereview.chromium.org/314913002/diff/20001/Tools/GardeningServer/sc...
Tools/GardeningServer/scripts/ui.js:424: Promise.all([
checkout.lastBlinkRollRevision(), rollbot.fetchCurrentRoll()
]).then(function(results) {
On 2014/06/05 00:54:48, ojan wrote:
> Nit: I don't know if we have an official style rule for this, but I think we
> normally wouldn't put the spaces after/before the [ and ]. The other places we
> do arrays we don't put the spaces.

Done.

https://codereview.chromium.org/314913002/diff/20001/Tools/GardeningServer/sc...
File Tools/GardeningServer/scripts/ui_unittests.js (right):

https://codereview.chromium.org/314913002/diff/20001/Tools/GardeningServer/sc...
Tools/GardeningServer/scripts/ui_unittests.js:228: {
"messages":[],"base_url":"svn://svn.chromium.org/chrome/trunk/src","subject":"Blink
roll 540:550","closed":false,"issue":1000}
On 2014/06/05 00:54:48, ojan wrote:
> Nit: We're a little less consitent about the spaces here, but most places
don't
> put the space after the { and do put spaces after the commas and colons.

Done.

https://codereview.chromium.org/314913002/diff/20001/Tools/GardeningServer/sc...
Tools/GardeningServer/scripts/ui_unittests.js:257:
equal(revisionDetails.innerHTML, 'Latest revision processed by every bot:
<details><summary><a
href="http://src.chromium.org/viewvc/blink?view=rev&amp;revision=554">554<span
id="revisionPopUp"><table><tr><td><a
href="http://build.chromium.org/p/chromium.webkit/waterfall?builder=Linux">Linux</a></td><td>554</td></tr></table></span></a></summary><table><tr><td><a
href="http://build.chromium.org/p/chromium.webkit/waterfall?builder=Linux">Linux</a></td><td>554</td></tr></table></details>,
trunk is at <a
href="http://src.chromium.org/viewvc/blink?view=rev&amp;revision=555">555</a><br>Last
roll is to <a
href="http://src.chromium.org/viewvc/blink?view=rev&amp;revision=540">540</a>,
current autoroll <a href="https://codereview.chromium.org/1000">540:550</a>');
On 2014/06/05 00:54:48, ojan wrote:
> When I do innerHTML based tests, I usually format the string so that it's
human
> readable, e.g.
> 
> 'Latest revision processed by every bot: ' +
>     '<details>' +
>         '<summary>' +
>             '<a
> href="http://src.chromium.org/viewvc/blink?view=rev&amp;revision=554">554' +
> ...
> 
> Alternately, instead of using innerHTML, I do:
> var container = document.createElement('div');
> container.innerHTML = revisionDetails.innerHTML;
> 
> var expectedUrls = [...];
> var expectedLinkText = [...];
> var links = container.querySelectorAll('a');
> // Loop through links and verify the URLs and link text all match
> // Verify anything else about the DOM that seems like valuable testing to you.
> 
> The advantage of the latter is that it makes it a bit easier to maintain, e.g.
> if you just add a space or a comma or something you don't need to update this
> test.

As we will be changing the formatting and it has an importance, I took the
innerHTML approach with a better format for the expected result..

[Julien - ping for review, 2014-06-05 01:52:46]

The CQ bit was checked by jchaffraix@chromium.org

[commit-bot: I haz the power, 2014-06-05 01:53:48]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jchaffraix@chromium.org/314913002/40001

[commit-bot: I haz the power, 2014-06-05 03:09:25]

Change committed as 175525