<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - Display failing JSC stress tests in buildbot dashboard"
href="https://bugs.webkit.org/show_bug.cgi?id=156595#c17">Comment # 17</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - Display failing JSC stress tests in buildbot dashboard"
href="https://bugs.webkit.org/show_bug.cgi?id=156595">bug 156595</a>
from <span class="vcard"><a class="email" href="mailto:dean_johnson@apple.com" title="Dean Johnson <dean_johnson@apple.com>"> <span class="fn">Dean Johnson</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=279694&action=diff" name="attach_279694" title="Patch">attachment 279694</a> <a href="attachment.cgi?id=279694&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=279694&action=review">https://bugs.webkit.org/attachment.cgi?id=279694&action=review</a>
If I were you, I'd also change all instances of "javaScriptCore" to "jsc" and "JavaScriptCore" to "JSC". I think it's well enough known what JSC represents to assume the reader will know what's happening in the code, but it'd probably be wise to get opinions from others before taking that suggestion.
Overall, it looks pretty good!
<span class="quote">> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotCombinedQueueView.js:114
> + var statusView = new StatusLineView(message, StatusLineView.Status.Bad, this._testStepFailureDescription(failedStep), failedStep.failureCount, mostRecentFinishedIteration.queue.buildbot.javaScriptCoreTestStdioURLForIteration(mostRecentFinishedIteration, failedStep.name));</span >
This line is really long and somewhat difficult to read. Can we store `mosRecentFinishedIteration.queue.buildbot.javascriptCoreTestStdioURLForIteration(mostRecentFinishedIteration, failedStep.name)` into a variable and use that as the argument instead of in-lining?
<span class="quote">> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotCombinedQueueView.js:143
> + var statusView = new StatusLineView(message, StatusLineView.Status.Good, firstRecentUnsuccessfulIteration ? "last succeeded" : statusMessagePassed, null, url);</span >
I realize that this existed before you edited it, but can we not inline the conditional and instead store it in a variable? It would significantly improve readability, in my opinion.
<span class="quote">> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotCombinedQueueView.js:147
> + this.element.appendChild(statusView.element);</span >
Ditto above
<span class="quote">> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:363
> + if (this.queue.buildbot.needsAuthentication && this.queue.buildbot.authenticationStatus === Buildbot.AuthenticationStatus.InvalidCredentials)</span >
Shouldn't this be this.queue.buildbot.needsAuthentication || this.queue.buildbot.authenticationStatus === Buildbot.AuthenticationStatus.InvalidCredentials?
<span class="quote">> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueueView.js:257
> + var elementType = (additionalTextTarget)? "a" : "span";</span >
Nit: Space after ")"</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>