[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

Dean Johnson <dean_johnson at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #267593|review?                     |review-
              Flags|                            |

--- Comment #7 from Dean Johnson <dean_johnson at apple.com> ---
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

Looks good overall. Few things, but otherwise nice work... I know a lot of people will be happy to see these changes merged. :)

Also, thanks for adding QUnit! It's awesome that we have a way to test our front-end now.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueue.js:301
>          var sortedRepositories = Dashboard.sortedRepositories;
>          for (var i = 0; i < sortedRepositories.length; ++i) {
>              var repositoryName = sortedRepositories[i].name;
> -            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;
> +            var result = trac.compareRevisions(b.revision[repositoryName], a.revision[repositoryName]);
>              if (result)
>                  return result;
>          }

Could compareIterations and compareIterationsByRevisions share this piece of code? It looks identical.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Trac.js:272
> +    compareRevisions: function(a, b)
> +    {
> +        var indexA = this.recordedCommits.objectIndexOf(a, "revisionNumber");
> +        var indexB = this.recordedCommits.objectIndexOf(b, "revisionNumber");
> +        if (indexA === undefined || indexB === undefined) {
> +            this.loadMoreHistoricalData();
> +            return undefined;
> +        }
> +        return indexA - indexB;
> +    },

This seems to make the assumption that a > b. Is it ever possible that b > a? If so, maybe use indexA as max(IndexA, indexB) and indexB as min(indexA, indexB).

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Utilities.js:170
> +{

I think this may make more sense as something like: indexOfObjectWithProperty. I don't like how verbose that is necessarily though, but it does seem a bit confusing without reading the function code. Ideas?

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Utilities.js:172
> +        if (this[i][property] === searchTerm) return i;

Nit: I think this would look a little nicer on two lines.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/tests/MockOverrides.js:5
> +        if (x) {

What is x? Could we use a better variable?

-- 
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/652e5b48/attachment.html>