[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 #9 from Jason Marcell <jmarcell at apple.com> ---
(In reply to comment #7)
> Comment on attachment 267593 [details]
> 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.

Yes, both compareIterations and compareIterationsByRevisions are very similar. The latter was introduced here:
https://bugs.webkit.org/show_bug.cgi?id=143885

We should probably find a way to collapse these into a single function if possible so that we are not duplicating code.

> 
> > 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).

This does not make the assumption that a > b.

This is a pretty common pattern used for things like sort functions. The expected API is as follows: negative return value if the first parameter is less than the second, zero if they’re equal, a positive return value otherwise.

See, for examples of this pattern, the following:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/sort 

> 
> > 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?

That doesn't seem like a bad name, but I'm open to other suggestions. I'll use that if there are no better suggestions.

> 
> > 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.

I'll add a line break.

> 
> > 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?

x is actually the value that the user passes to the setter. Should we use 'value' as the variable name instead?

Some background: My intention for the MockOverrides.js file was to provide a place where we would override class methods (rather than creating a derived Mock subclass). The idea is that you want to cherry-pick some functionality in a particular method of some class while leaving some other functionality out. To that end, I copied this code from the original setter (bad variable name and all) while leaving out some code that getting in the way of my testing. I know copy/paste code is usually a bad idea, but it's going to help with testing :/

And if you're wondering why I overrode the method in the original class instead of just creating a Mock subclass that overrides the method, this is a class that I'm not directly instantiating myself, so I'm not in a position to provide a Mock class as a substitute.

-- 
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/86b2775d/attachment-0001.html>