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

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has denied 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 136381: Patch
https://bugs.webkit.org/attachment.cgi?id=136381&action=review

------- Additional Comments from Daniel Bates <dbates at webkit.org>
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.