[Webkit-unassigned] [Bug 152345] Teach dashboard code to compare non-integer revisions.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=152345

--- Comment #34 from Daniel Bates <dbates at webkit.org> ---
Comment on attachment 270708
  --> https://bugs.webkit.org/attachment.cgi?id=270708
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=270708&action=review

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueue.js:292
> +            if (indexA > -1 && indexB > -1) {

This is OK as-is. I suggest explicitly comparing against -1.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueueView.js:172
> +            var nextRevision = trac.nextRevision(branch.name, latestProductiveRevisionNumber);
> +            if (!latestProductiveRevisionNumber || !trac.latestRecordedRevisionNumber || !nextRevision)
>                  continue;

This is very minor. I suggest we compute nextRevision and early return (if necessary) after line 171.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueueView.js:237
>              if (context.firstRevision <= context.lastRevision)

This is incorrect for Git SHAs, which are not numeric.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Trac.js:96
> +        console.assert(indexOfLastRevision !== -1, indexOfLastRevision + " is not in the list of recorded commits");

indexOfLastRevision => lastRevision

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Trac.js:304
> +        return undefined;

I suggest either returning null or we may want to define a named constant (it could equal null), maybe Trac.NO_MORE_REVISIONS, if we feel this may improve the readability at the call site.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Trac.js:310
> +        for (var i = 0; i < commits.length; ++i)

Nit: Missing braces. We add braces whenever the body of the control-flow statement is more than one line.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/tests/tests.js:167
> +        return {
> +            revision: {
> +                "openSource": 33021
> +            }
> +        };

This is OK as-is. I would have written this as:

var iteration = {
    revision: { "openSource": 33021 },
};
return iteration;

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20160205/b4ec6ef2/attachment.html>