Bug 83541 – Add a chromeless view to the individual tests view

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Ojan Vafai]

Add a chromeless view to the individual tests view

[Ojan Vafai]

Created attachment 136381 [details]
Patch

[Daniel Bates]

Comment on attachment 136381 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=136381&action=review

This patch doesn't look correct.

> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:1819
>          master = builderMaster(testResults[0].builder);

Did you intend to make the variable master global? Because this patch removed the declaration "var master" (above) this line will create a global variable called master. From looking at the names of other global variables in declared in this file, a global variable should have a "g_" prefix.

> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:1840
> +    var master;

This variable is unused and masks the global variable of the same name defined in htmlForIndividulTestOnAllBuilders() (above). Hence, the equality check "master == WEBKIT_BUILDER_MASTER" (in the original code; not part of this patch) will always evaluate to false, since |master| === undefined.

[Daniel Bates]

(In reply to comment #2)
> > Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:1840
> > +    var master;
> 
> This variable is unused and masks the global variable ....

Disregard the "This variable is unused" part of this sentence.

[Ojan Vafai]

Created attachment 136503 [details]
Patch

[Ojan Vafai]

(In reply to comment #2)
> (From update of attachment 136381 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=136381&action=review
> 
> This patch doesn't look correct.
> 
> > Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:1819
> >          master = builderMaster(testResults[0].builder);
> 
> Did you intend to make the variable master global? Because this patch removed the declaration "var master" (above) this line will create a global variable called master. From looking at the names of other global variables in declared in this file, a global variable should have a "g_" prefix.

Whoops. No. I just missed this. Inlined the variable and added test cases. Thanks for catching this.

Also, I made the postMessage code a little more robust.

[Daniel Bates]

Comment on attachment 136503 [details]
Patch

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]));

[Adam Barth]

Comment on attachment 136503 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=136503&action=review

> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:2608
> +        event.source.postMessage('{"height":' + document.documentElement.offsetHeight + '}', '*');

Can we use JSON.stringify rather than building up JSON strings with string concatenation?

[Ojan Vafai]

Comment on attachment 136503 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=136503&action=review

>> 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].

Good eye. The old code was wrong. The list cannot be empty. I'll changed the other code to remove the length check.

>> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:2405
>> +    }
> 
> 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]));

Good suggestion.

>> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:2608
>> +        event.source.postMessage('{"height":' + document.documentElement.offsetHeight + '}', '*');
> 
> Can we use JSON.stringify rather than building up JSON strings with string concatenation?

Good idea.

[Ojan Vafai]

Committed r113787: <http://trac.webkit.org/changeset/113787>