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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Feb 8 10:38:18 PST 2016


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

--- Comment #37 from Jason Marcell <jmarcell at apple.com> ---
(In reply to comment #34)
> Comment on attachment 270708 [details]
> 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.

Done.

> 
> > 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.

Done.

> 
> > 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.

I addressed this.

> 
> > 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.

I went with Tac.NO_MORE_REVISIONS and defined it as 'null'.

> 
> > 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.

Fixed.

> 
> > 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;

Fixed.

-- 
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/20160208/fb793200/attachment-0001.html>


More information about the webkit-unassigned mailing list