<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#c29">Comment # 29</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:jmarcell@apple.com" title="Jason Marcell <jmarcell@apple.com>"> <span class="fn">Jason Marcell</span></a>
</span></b>
<pre>(In reply to <a href="show_bug.cgi?id=152345#c26">comment #26</a>)
<span class="quote">> 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>
>
> > 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;</span >
What if endPosition is 0? Maybe we should instead do something like:
if (endPosition === undefined)
endPosition = this.recordedCommits.length - 1;
<span class="quote">> let commits = [];</span >
Why are you using 'let' instead of 'var'?
<span class="quote">> 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.</span ></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>