Make the legend show on '?' keypresses and hide on escape. Make it hidden...

Make the legend show on '?' keypresses and hide on escape. Make it hidden
by default.

Show expectations for test. Show the test and all possible expectations for it.
Identify which expectations are used on which platform. Once we store the raw
files for the latest run on the bots, we can also show the actual results.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=28069

[arv (Not doing code reviews), 2009-10-05 22:19:06]

http://codereview.chromium.org/243104/diff/1/2
File webkit/tools/layout_tests/flakiness_dashboard.html (right):

http://codereview.chromium.org/243104/diff/1/2#newcode1281
Line 1281: if (path.match(/\\$/))
What if the path ends with /? Right now you are only checking for backslash

http://codereview.chromium.org/243104/diff/1/2#newcode1294
Line 1294: * @param {object} expectationsContainers Map from expectations type
to
Object

http://codereview.chromium.org/243104/diff/1/2#newcode1309
Line 1309: var isImage = path.match(/^.*\.png$/);
Isn't this the same as /\.png$/ ?

http://codereview.chromium.org/243104/diff/1/2#newcode1312
Line 1312: // tags? They spew a lot of errors to the console.
What if the expectation is a script that has side effects like alert? Would that
screw things up?

http://codereview.chromium.org/243104/diff/1/2#newcode1312
Line 1312: // tags? They spew a lot of errors to the console.
Did you try using <img> for everything?

http://codereview.chromium.org/243104/diff/1/2#newcode1388
Line 1388: winner.getElementsByClassName('platforms')[0].innerHTML +=
winner can be undefined or null here

http://codereview.chromium.org/243104/diff/1/2#newcode1507
Line 1507: * Returns the reverse sorted, deduped list of all platform fallback
directories.
long line

http://codereview.chromium.org/243104/diff/1/2#newcode1726
Line 1726: document.addEventListener('keyup', function(e) {
did you mean keydown?

http://codereview.chromium.org/243104/diff/1/2#newcode1727
Line 1727: if (e.keyCode == 191 && e.shiftKey) {
keyIdentifier is your friend

[ojan, 2009-10-05 22:55:30]

All done. Thanks for the quick review.

http://codereview.chromium.org/243104/diff/1/2
File webkit/tools/layout_tests/flakiness_dashboard.html (right):

http://codereview.chromium.org/243104/diff/1/2#newcode1281
Line 1281: if (path.match(/\\$/))
On 2009/10/05 22:19:06, arv wrote:
> What if the path ends with /? Right now you are only checking for backslash

Doh. Fixed. Turned out not to matter. I'm leaving it in though for
future-proofness.

http://codereview.chromium.org/243104/diff/1/2#newcode1294
Line 1294: * @param {object} expectationsContainers Map from expectations type
to
On 2009/10/05 22:19:06, arv wrote:
> Object

Done.

http://codereview.chromium.org/243104/diff/1/2#newcode1309
Line 1309: var isImage = path.match(/^.*\.png$/);
On 2009/10/05 22:19:06, arv wrote:
> Isn't this the same as /\.png$/ ?

Done.

http://codereview.chromium.org/243104/diff/1/2#newcode1312
Line 1312: // tags? They spew a lot of errors to the console.
On 2009/10/05 22:19:06, arv wrote:
> Did you try using <img> for everything?

Yes. Onload never fired for text results (onerror did).  If I had control over
the servers, I might be able to trick it by sending an image content-type, but
that seems hacky anyways.

Not sure if it would work, but Julie suggested loading all the script tags in an
iframe and listening to window.onerror on the iframe's window. Would that hide
the errors?

http://codereview.chromium.org/243104/diff/1/2#newcode1312
Line 1312: // tags? They spew a lot of errors to the console.
On 2009/10/05 22:19:06, arv wrote:
> What if the expectation is a script that has side effects like alert? Would
that
> screw things up?

Probably.

http://codereview.chromium.org/243104/diff/1/2#newcode1388
Line 1388: winner.getElementsByClassName('platforms')[0].innerHTML +=
On 2009/10/05 22:19:06, arv wrote:
> winner can be undefined or null here

Logging an error now. This should only happen if there's a logic bug in the
dashboard, or if trac.webkit.org or src.chromium.org serves 5XXs for some
expectations.

http://codereview.chromium.org/243104/diff/1/2#newcode1507
Line 1507: * Returns the reverse sorted, deduped list of all platform fallback
directories.
On 2009/10/05 22:19:06, arv wrote:
> long line

Done.

http://codereview.chromium.org/243104/diff/1/2#newcode1726
Line 1726: document.addEventListener('keyup', function(e) {
On 2009/10/05 22:19:06, arv wrote:
> did you mean keydown?

Done.

http://codereview.chromium.org/243104/diff/1/2#newcode1727
Line 1727: if (e.keyCode == 191 && e.shiftKey) {
On 2009/10/05 22:19:06, arv wrote:
> keyIdentifier is your friend

Done. Although, I'm not sure that's better. There's a discussion on www-dom
about making keyIdentifier more sane for ascii (e.g. actually return '?' instead
of 'U+003F"...or at least return '\u003f').

[arv (Not doing code reviews), 2009-10-05 23:23:48]

http://codereview.chromium.org/243104/diff/1/2
File webkit/tools/layout_tests/flakiness_dashboard.html (right):

http://codereview.chromium.org/243104/diff/1/2#newcode1312
Line 1312: // tags? They spew a lot of errors to the console.
On 2009/10/05 22:55:30, ojan wrote:
> On 2009/10/05 22:19:06, arv wrote:
> > Did you try using <img> for everything?
> 
> Yes. Onload never fired for text results (onerror did).  If I had control over
> the servers, I might be able to trick it by sending an image content-type, but
> that seems hacky anyways.

If you had access of the servers you could used access control header

> Not sure if it would work, but Julie suggested loading all the script tags in
an
> iframe and listening to window.onerror on the iframe's window. Would that hide
> the errors?

window.onerror is not implemented in webkit

I guess this solution is the only way to achive this at the moment.

http://codereview.chromium.org/243104/diff/5/1002#newcode1738
Line 1738: </head>
I thought 'Esc' was valid? If that is not the case then keyCode seems more
appropriate

[ojan, 2009-10-05 23:34:56]

http://codereview.chromium.org/243104/diff/5/1002
File webkit/tools/layout_tests/flakiness_dashboard.html (right):

http://codereview.chromium.org/243104/diff/5/1002#newcode1738
Line 1738: } else if (e.keyIdentifier == 'U+001B') {
On 2009/10/05 23:23:48, arv wrote:
> I thought 'Esc' was valid? If that is not the case then keyCode seems more
> appropriate

There's a big discussion on www-dom about this. I think, although I'm not sure,
the conclusion was the keyIdentifier should return things like 'Esc', but right
now, in Chrome (haven't tested Safari/Firefox), that's not what it returns.

Switched back to keycode.

[arv (Not doing code reviews), 2009-10-05 23:59:29]

LGTM

I would probably use keyIdentifier after all since checking the shift key is
clearly wrong.

http://codereview.chromium.org/243104/diff/1003/6
File webkit/tools/layout_tests/flakiness_dashboard.html (right):

http://codereview.chromium.org/243104/diff/1003/6#newcode1736
Line 1736: // ? key
This will not work on different keyboard layouts. Man, keyboard events suck!

At least keyIdentifier is different for / and ?