[webkit-reviews] review denied: [Bug 136386] Update webkit dashboard to support performance bots : [Attachment 237383] Implementation to display performance bots on the dashboard

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied	review:
Bug 136386: Update webkit dashboard to support performance bots
https://bugs.webkit.org/show_bug.cgi?id=136386

Attachment 237383: Implementation to display performance bots on the dashboard
https://bugs.webkit.org/attachment.cgi?id=237383&action=review

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


Dana and myself checked whether this new column fits on a 13" screen, and it
doesn't quite. Fortunately, the layout can be squeezed very very slightly to
make it fit, but we can't have any more columns (which is a bummer, as there
are also leaks bots and other misc ones).

>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/B
uildbot.js:62
> +    Performance: "perf"

We avoid abbreviations in WebKit code base. Please change all "perf"s into
complete words.

>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/B
uildbotTesterQueueView.js:128
> +		   if (queue.perfType) {
> +		       label = queue.perfType;
> +		   }

There are no braces around single line blocks in WebKit coding style.

>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/B
uildbotTesterQueueView.js:146
> +	   if (this.perfQueues) {
> +	       appendBuild.call(this, this.perfQueues);
> +	   }

Ditto. Also, this check is always true anyway, as [] converts to true.

>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/M
ain.js:56
>	   var buildType = queue.debug ? "debug" : "release";
> +	   buildType = queue.perfType ? "perfType" : buildType;

I'd probably use a nested conditional here - it's unnecessarily confusing to
initialize a variable and immediately override it. Not a big deal though.

>>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/W
ebKitBuildbot.js:54
>> +	    "EFL Linux 64-bit Release WK2 (Perf)": {platform:
Dashboard.Platform.LinuxEFL, tester: true, testCategory:
Buildbot.TestCategory.Performance}
> 
> These seem unrelated to the patch (although I suppose it's okay to add them
now while you're in the neighborhood).

OK to land, but I'd still suggest splitting this into a separate patch.