[webkit-reviews] review granted: [Bug 127006] build.webkit.org/dashboard should display information about patches in EWS : [Attachment 221624] proposed patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted Alexey Proskuryakov
<ap at webkit.org>'s request for review:
Bug 127006: build.webkit.org/dashboard should display information about patches
in EWS
https://bugs.webkit.org/show_bug.cgi?id=127006

Attachment 221624: proposed patch
https://bugs.webkit.org/attachment.cgi?id=221624&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=221624&action=review


>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/B
ugzilla.js:2
> + * Copyright (C) 2013 Apple Inc. All rights reserved.

Nit: should read 2014 or 2013, 2014.

>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/E
WSQueue.js:123
> +		   // The server returns all bots that have ever checked in
with it. We only need currently active bots.
> +		   var oneDayInMilliseconds = 24 * 60 * 60 * 1000;
> +		   if (latestMessageTime < Date.now() - oneDayInMilliseconds)

I would prefer declaring a local variable named like botIsCurrentlyActive
instead of having a comment like this.

>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/E
WSQueueView.js:128
> +	   var hours = Math.floor(timeDifference / (60 * 60));
> +	   var minutes = Math.floor((timeDifference - hours * 60 * 60) / 60);

Can we define a constant like secondsPerHour?

>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/E
WSQueueView.js:155
> +	       this.addLinkToRow(rowElement, "patch-details-link",
patch.attachmentID, patch.statusPageURL);

Looks like the code block below could be extracted as a helper function.