<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#c9">Comment # 9</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#c7">comment #7</a>)
<span class="quote">> Comment on <span class=""><a href="attachment.cgi?id=267593&action=diff" name="attach_267593" title="Patch">attachment 267593</a> <a href="attachment.cgi?id=267593&action=edit" title="Patch">[details]</a></span>
> Patch
>
> View in context:
> <a href="https://bugs.webkit.org/attachment.cgi?id=267593&action=review">https://bugs.webkit.org/attachment.cgi?id=267593&action=review</a>
>
> 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.</span >
Yes, both compareIterations and compareIterationsByRevisions are very similar. The latter was introduced here:
<a class="bz_bug_link
bz_status_RESOLVED bz_closed"
title="RESOLVED FIXED - build.webkit.org/dashboard still shows obsolete results for out of order builds sometimes"
href="show_bug.cgi?id=143885">https://bugs.webkit.org/show_bug.cgi?id=143885</a>
We should probably find a way to collapse these into a single function if possible so that we are not duplicating code.
<span class="quote">>
> > 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).</span >
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:
<a href="https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/sort">https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/sort</a>
<span class="quote">>
> > 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?</span >
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.
<span class="quote">>
> > 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.</span >
I'll add a line break.
<span class="quote">>
> > 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?</span >
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.</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>