[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 #26 from Daniel Bates <dbates at webkit.org> ---
Comment on attachment 270457
  --> https://bugs.webkit.org/attachment.cgi?id=270457
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;
    let commits = [];
    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/20160203/1fb70e5b/attachment-0001.html>