[webkit-reviews] review granted: [Bug 101976] garden-o-matic doesn't know about reftests : [Attachment 178929] Patch

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has granted Dirk Pranke <dpranke at chromium.org>'s
request for review:
Bug 101976: garden-o-matic doesn't know about reftests
https://bugs.webkit.org/show_bug.cgi?id=101976

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

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


>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/script
s/controllers.js:44
> +	   if (isAnyReftest(testName, resultsByTest)) {
> +	       statusView.addMessage(id, testName + ' is a ref test,
skipping');
> +	   } else {

Nit: single-line if shouldn't have curlies.

>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/script
s/controllers.js:60
> +    } else {
> +	   statusView.addFinalMessage(id, 'No tests left to rebaseline!')
> +    }

Nit: single-line else shouldn't have curlies.

Non-nit: maybe make this text more explict? 'No non-reftests to rebaseline.'

>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/script
s/controllers.js:63
> +// FIXME: This is duplicated from ui/results.js :(.

ui.results.js is probably the wrong place for it. Maybe it should just be in
results.js (i.e. not in the ui directory) as results.isAnyReftest. Then this
code and ui.results.js can both call it? I dunno. I don't have the intended
architecture of this stuff fresh in my brain, so not sure if that makes sense.

This is fine for now w the FIXME.