[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 #8 from Daniel Bates <dbates at webkit.org> ---
Comment on attachment 267593
  --> https://bugs.webkit.org/attachment.cgi?id=267593
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=267593&action=review

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueue.js:276
> -            var result = b.revision[repositoryName] - a.revision[repositoryName];
> +            var trac = sortedRepositories[i].trac;
> +            if (!trac || b.revision[repositoryName] === undefined || a.revision[repositoryName] === undefined)
> +                return undefined;

Can you give an example where we would not have a revision for a repository?

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueue.js:298
> +            var trac = sortedRepositories[i].trac;
> +            if (!trac || b.revision[repositoryName] === undefined || a.revision[repositoryName] === undefined)
> +                return undefined;
> +            var result = trac.compareRevisions(b.revision[repositoryName], a.revision[repositoryName]);

Ditto.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueueView.js:95
> +            if (!trac.latestRecordedRevisionNumber || comparison === undefined || comparison > 0) {

Can you give an example when comparison === undefined?

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueueView.js:100
> +            totalRevisionsBehind += trac.commitsOnBranch(branch.name, function(commit, index, arr) { return index > arr.objectIndexOf(latestProductiveRevisionNumber, "revisionNumber"); }).length;

The name arr is not a good name for the array of commits. I am assuming arr is short for "array". Regardless, neither "arr" nor "array" are good names because the former is an abbreviation of the latter (and I do not find it more canonical than the word "array") and the latter is a generic term that does not convey anything about the contents of the array. Maybe a better name would be commits or allCommits?

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueueView.js:145
> +        var commits = trac.commitsOnBranch(branch.name, function(commit, index, arr) { return index >= arr.objectIndexOf(firstRevisionNumber, "revisionNumber") && index <= arr.objectIndexOf(lastRevisionNumber, "revisionNumber"); });

Ditto.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Trac.js:68
> +        return this.recordedCommits.filter(function(commit, index, arr) {

Ditto.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Trac.js:266
> +        var indexA = this.recordedCommits.objectIndexOf(a, "revisionNumber");
> +        var indexB = this.recordedCommits.objectIndexOf(b, "revisionNumber");

Can you elaborate on when a commit would not have a revision?

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Trac.js:269
> +            this.loadMoreHistoricalData();
> +            return undefined;

This does not seem like the correct approach. Having a comparison function fetch more data and have a polymorphic return type is weird.

-- 
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/20151218/c69cabef/attachment.html>