[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 #29 from Jason Marcell <jmarcell at apple.com> ---
(In reply to comment #26)
> Comment on attachment 270457 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=270457&action=review
> 
> > Tools/ChangeLog:20
> > +        (Trac.prototype.commitsOnBranchLaterThanRevision): Finds revisions on a branch great later than the specified revision.
> 
> Nit: This sentence does not read well.
> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueue.js:290
> > +            indexA = trac.indexOfRevision(a.revision[repositoryName]);
> 
> This will define a global variable named indexA. We should make this a local
> variable as you did for variable trac.
> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueue.js:291
> > +            indexB = trac.indexOfRevision(b.revision[repositoryName]);
> 
> Ditto.
> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Trac.js:73
> > +    _commitsOnBranch: function(branchName, filter)
> >      {
> > +        if (!filter)
> > +            filter = function(commit) { return true; };
> >          return this.recordedCommits.filter(function(commit, index, array) {
> >              return (!commit.containsBranchLocation || commit.branches.includes(branchName)) && filter(commit, index, array);
> >          });
> >      },
> 
> We always pass a filter to this function and that filter only looks at the
> index position. So, I suggest we remove the filter argument and have this
> function take optional begin and end index positions:
> 
> _commitsOnBranch(branchName, beginPosition, endPosition)
> {
>     beginPosition = beginPosition || 0;
>     endPosition = endPosition || this.recordedCommits.length - 1;

What if endPosition is 0? Maybe we should instead do something like:

if (endPosition === undefined)
    endPosition = this.recordedCommits.length - 1;

>     let commits = [];

Why are you using 'let' instead of 'var'?

>     for (let i = beginPosition; i <= endPosition; ++i) {
>         let commit = this.recordedCommits[i]
>         if (!commit.containsBranchLocation ||
> commit.branches.includes(branchName))
>             commits.push(commit);
>     }
>     return commits;
> },
> 
> This will allow us to remove the need to iterate over the entire array of
> recorded commits once we know either a begin or end position (or both).
> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Trac.js:82
> > +        var indexToBeLaterThan = this.indexOfRevision(revision);
> > +        function selectCommitsLaterThanRevision(commit, index, allCommits) {
> > +            return index > indexToBeLaterThan;
> > +        }
> > +        var commits = this._commitsOnBranch(branchName, selectCommitsLaterThanRevision.bind(this));
> > +        return commits;
> 
> Then we can implement this as:
> 
> let indexToBeLaterThan = this.indexOfRevision(revision);
> console.assert(indexToBeLaterThan !== -1, revision + " is not in the list of
> recorded commits");
> if (indexToBeLaterThan === -1)
>     return [];
> return this._commitsOnBranch(branchName, null, indexToBeLaterThan + 1);
> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Trac.js:93
> > +        var indexOfFirstRevision = this.indexOfRevision(firstRevision);
> > +        var indexOfLastRevision = this.indexOfRevision(lastRevision);
> > +        function selectCommitsInRange(commit, index, allCommits) {
> > +            return index >= indexOfFirstRevision && index <= indexOfLastRevision;
> > +        }
> > +        var commits = this._commitsOnBranch(branchName, selectCommitsInRange.bind(this));
> > +        return commits;
> 
> And we can implement this as:
> 
> let indexOfFirstRevision = this.indexOfRevision(firstRevision);
> console.assert(indexOfFirstRevision !== -1, firstRevision + " is not in the
> list of recorded commits");
> if (indexOfFirstRevision === -1)
>     return [];
> let indexOfLastRevision = this.indexOfRevision(lastRevision);
> console.assert(indexOfLastRevision !== -1, indexOfLastRevision + " is not in
> the list of recorded commits");
> if (indexOfLastRevision === -1)
>     return [];
> return this._commitOnBranch(branchName, null, indexOfFirstRevision,
> indexOfLastRevision);
> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Utilities.js:177
> > +Array.prototype.indexOfObjectPassingTest = function(predicate)
> > +{
> > +    for (var i = 0; i < this.length; ++i) {
> > +        if (predicate(this[i]))
> > +            return i;
> > +    }
> > +
> > +    return -1;
> > +};
> 
> This function is only used by Trac.indexOfRevision(). Unless you plan to
> make use of this function in a near future patch, I suggest that we inline
> the implementation of this function into Trac.indexOfRevision() and remove
> this function.

-- 
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/20160204/700522cc/attachment.html>