[webkit-reviews] review granted: [Bug 136386] Update webkit dashboard to support performance bots : [Attachment 237540] Updated patch with Alexey's suggestions.

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Dana Burkart
<dburkart at apple.com>'s request for review:
Bug 136386: Update webkit dashboard to support performance bots
https://bugs.webkit.org/show_bug.cgi?id=136386

Attachment 237540: Updated patch with Alexey's suggestions.
https://bugs.webkit.org/attachment.cgi?id=237540&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=237540&action=review


> I can change it in both places if you want.

Yes, I think that comparing to BuildbotIteration.SUCCESS reads better. The
existing place probably pre-dates adding these named constants.

> Tools/ChangeLog:18
> +	   Support new 'performance' and 'perfType' keys.

Stale ChangeLog, perfType is no more.

>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/B
uildbotIteration.js:259
> +	       testResults.failureCount = (testStep.results[0]) ? 1 : 0;

I think that this is not quite how the other result objects work. Failure count
is for steps that actually have failure counts (like layout tests).

Besides, what are the possible values for testStep.results[0] here? We know
that it's not 0, because that case is handled above. How else can
testStep.results[0] evaluate to false? This needs a comment if that can
actually happen.

>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/B
uildbotIteration.js:315
> +	   // This is actually correct, we're calling collectTestResults vs.
collectPerformanceTestResults because of
> +	   // differences in the internal buildbot.

"This is actually correct" is superfluous, you can just say "We are calling
collectTestResults and not collectPerformanceTestResults because of differences
in the internal buildbot."

A little more detail about what the differences are could be appropriate (cf.
how we explain revision number formats for internal bots with two source
trees).

>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/B
uildbotPerformanceQueueView.js:91
> +		   releaseLabel.textContent = queue.performanceType ?
queue.performanceType : label;

This still seems a little confusing. What is "performance type", that doesn't
parse.

Perhaps performanceTestName would be more descriptive?

>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/B
uildbotPerformanceQueueView.js:116
> +	   content.className = "ews-popover";

Didn't notice this at first - since it's not a EWS popover, please don't use
its class names. Duplicating style rules is OK.

>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/W
ebKitBuildbot.js:66
> +    performanceDashboardURL: function()

I now think that I gave the wrong advice here. I was thinking of a getter
function, but we have regular functions when there is an argument, and simple
attributes like baseURL when there is none.