<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - Teach dashboard code to compare non-integer revisions."
href="https://bugs.webkit.org/show_bug.cgi?id=152345#c26">Comment # 26</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - Teach dashboard code to compare non-integer revisions."
href="https://bugs.webkit.org/show_bug.cgi?id=152345">bug 152345</a>
from <span class="vcard"><a class="email" href="mailto:dbates@webkit.org" title="Daniel Bates <dbates@webkit.org>"> <span class="fn">Daniel Bates</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=270457&action=diff" name="attach_270457" title="Patch">attachment 270457</a> <a href="attachment.cgi?id=270457&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=270457&action=review">https://bugs.webkit.org/attachment.cgi?id=270457&action=review</a>
<span class="quote">> Tools/ChangeLog:20
> + (Trac.prototype.commitsOnBranchLaterThanRevision): Finds revisions on a branch great later than the specified revision.</span >
Nit: This sentence does not read well.
<span class="quote">> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueue.js:290
> + indexA = trac.indexOfRevision(a.revision[repositoryName]);</span >
This will define a global variable named indexA. We should make this a local variable as you did for variable trac.
<span class="quote">> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueue.js:291
> + indexB = trac.indexOfRevision(b.revision[repositoryName]);</span >
Ditto.
<span class="quote">> 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);
> });
> },</span >
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).
<span class="quote">> 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;</span >
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);
<span class="quote">> 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;</span >
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);
<span class="quote">> 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;
> +};</span >
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.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>