Summary: | Add a chromeless view to the individual tests view | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ojan Vafai <ojan> | ||||||
Component: | New Bugs | Assignee: | Ojan Vafai <ojan> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | abarth, dbates, eric | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Ojan Vafai
2012-04-09 19:53:09 PDT
Created attachment 136381 [details]
Patch
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. (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. Created attachment 136503 [details]
Patch
(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. 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])); 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? 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. Committed r113787: <http://trac.webkit.org/changeset/113787> |