[webkit-reviews] review denied: [Bug 64141] Teach garden-o-matic how to display test results : [Attachment 100062] Patch

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has denied Adam Barth <abarth at webkit.org>'s
request for review:
Bug 64141: Teach garden-o-matic how to display test results
https://bugs.webkit.org/show_bug.cgi?id=64141

Attachment 100062: Patch
https://bugs.webkit.org/attachment.cgi?id=100062&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=100062&action=review


Yay for tests!

> Tools/ChangeLog:7
> +

Wouldn't hurt to give a bit more description here, e.g. as to how this all
works. The code isn't especially hard for me to understand given it's ancestry,
but the way it works at a high-level is maybe not immediately obvious.

> Tools/Scripts/webkitpy/tool/servers/data/gardeningserver/base.js:50
> +base.probe = function(url, options)

s/options/handlers? and maybe change success->onsuccess and error->onerror?

> Tools/Scripts/webkitpy/tool/servers/data/gardeningserver/base.js:55
> +	   if ($.isFunction(options.success))

checking that it's a function seems excessive to me. bulks up the code. I think
it should be sufficient to just check "if (options.success)"

> Tools/Scripts/webkitpy/tool/servers/data/gardeningserver/base.js:56
> +	       options.success.call();

Can't you just call options.success()?

> Tools/Scripts/webkitpy/tool/servers/data/gardeningserver/base.js:60
> +	   if ($.isFunction(options.error))

ditto: if (options.error)

> Tools/Scripts/webkitpy/tool/servers/data/gardeningserver/base.js:61
> +	       options.error.call();

options.error()?

> Tools/Scripts/webkitpy/tool/servers/data/gardeningserver/index.html:54
> +.results iframe {
> +  width: 400px;
> +  height: 300px;
> +}
> +.results img {
> +  width: 400px;
> +  height: 300px;
> +}

.results iframe,
.results img {
...
}

> Tools/Scripts/webkitpy/tool/servers/data/gardeningserver/results.js:13
> +var kPossibleSuffixList = [

Because of reftests, there are -expected.html and -expected-mismatch.html and
for audio tests you'll need -expected.wav and -actual.wav.

You might want to look at
http://trac.webkit.org/browser/trunk/LayoutTests/fast/harness/results.html as a
reference for some of this stuff.

> Tools/Scripts/webkitpy/tool/servers/data/gardeningserver/results.js:132
> +results.resultKind = function(url)
> +{
> +    if (/-actual\.[a-z]+$/.test(url))
> +	   return 'actual';
> +    else if (/-expected\.[a-z]+$/.test(url))
> +	   return 'expected';
> +    else if (/diff\.[a-z]+$/.test(url))
> +	   return 'diff';
> +    return 'unknown';
> +}
> +
> +results.resultType = function(url)
> +{
> +    if (/\.png$/.test(url))
> +	   return 'image';
> +    return 'text';

Would prefer for these strings to be constants.

> Tools/Scripts/webkitpy/tool/servers/data/gardeningserver/results.js:135
> +function sortResultURLs(urls)

I'm confused by this name. This looks more like it filters than sorts.