[webkit-reviews] review granted: [Bug 83541] Add a chromeless view to the individual tests view : [Attachment 136503] Patch

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has granted Ojan Vafai <ojan at chromium.org>'s
request for review:
Bug 83541: Add a chromeless view to the individual tests view
https://bugs.webkit.org/show_bug.cgi?id=83541

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

------- Additional Comments from Daniel Bates <dbates at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=136503&action=review


Thanks Ojan for updating the patch. I noticed some minor issues.

> Tools/ChangeLog:13
> +	   (testHtmlForIndividulTestOnAllBuilders):
> +	   (testHtmlForIndividulTestOnAllBuildersWithChrome):

This change log needs to be updated. In particular, these functions have been
renamed to testHtmlForIndividualTestOnAllBuilders and
testHtmlForIndividualTestOnAllBuildersWithChrome, respectively.

> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:1855
> +	   if (testResults && builderMaster(testResults[0].builder) ==
WEBKIT_BUILDER_MASTER) {

Can testResults be an empty list? Is checking for isLayoutTestResults()
sufficient? Previously we checked for a non-zero value for testResults.length
before accessing testResults[0].

> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:2405
> +    for (var i = 0; i < tests.length; i++) {
> +	   var test = tests[i];
> +	   var html = g_currentState.showChrome ?
htmlForIndividualTestOnAllBuildersWithChrome(test) :
htmlForIndividualTestOnAllBuilders(test)
> +	   testsHTML.push(html);
> +    }

As far as I can tell the g_currentState.showChrome won't change inside this
loop. And we use the value of g_currentState.showChrome to determine which
function to call. I suggest hoisting this code outside of the for-loop and
inlining the definition of test such that this for-loop has the form:

var htmlForTestFunction = g_currentState.showChrome ?
htmlForIndividualTestOnAllBuildersWithChrome :
htmlForIndividualTestOnAllBuilders;
for (var i = 0; i < tests.length; i++)
    testsHTML.push(htmlForTestFunction(tests[i]));