<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><span class="vcard"><a class="email" href="mailto:dbates@webkit.org" title="Daniel Bates <dbates@webkit.org>"> <span class="fn">Daniel Bates</span></a>
</span> changed
<a class="bz_bug_link
bz_status_NEW "
title="NEW - Refactor to convert openSourceRevision and internalRevision properties on BuildbotIteration into a more generic collection of revisions"
href="https://bugs.webkit.org/show_bug.cgi?id=147280">bug 147280</a>
<br>
<table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>What</th>
<th>Removed</th>
<th>Added</th>
</tr>
<tr>
<td style="text-align:right;">Attachment #257486 Flags</td>
<td>review?
</td>
<td>review-
</td>
</tr></table>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - Refactor to convert openSourceRevision and internalRevision properties on BuildbotIteration into a more generic collection of revisions"
href="https://bugs.webkit.org/show_bug.cgi?id=147280#c2">Comment # 2</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - Refactor to convert openSourceRevision and internalRevision properties on BuildbotIteration into a more generic collection of revisions"
href="https://bugs.webkit.org/show_bug.cgi?id=147280">bug 147280</a>
from <span class="vcard"><a class="email" href="mailto:dbates@webkit.org" title="Daniel Bates <dbates@webkit.org>"> <span class="fn">Daniel Bates</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=257486&action=diff" name="attach_257486" title="Refactor to convert openSourceRevision and internalRevision properties on BuildbotIteration into a more generic collection of revisions.">attachment 257486</a> <a href="attachment.cgi?id=257486&action=edit" title="Refactor to convert openSourceRevision and internalRevision properties on BuildbotIteration into a more generic collection of revisions.">[details]</a></span>
Refactor to convert openSourceRevision and internalRevision properties on BuildbotIteration into a more generic collection of revisions.
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=257486&action=review">https://bugs.webkit.org/attachment.cgi?id=257486&action=review</a>
This patch looks good. I would like to see another iteration of it.
<span class="quote">> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:200
> // revision. Therefore, we only look at got_revision to extract the Internal revision when it's
> // a dictionary.</span >
The second paragraph of this comment is inconsistent with the code given the proposed changes. We should either update the second paragraph of the comment (if we feel it has value) or remove it.
<span class="quote">> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:207
> + var key = repository;
> + var fallbackKey = "Internal";</span >
For your consideration, I suggest we make fallbackKey null by default because the use of a fallback key is specific to the OpenSource and Internal repositories. You may also want to consider declaring key and fallbackKey here and moving there definition into an else clause of the if-else block below such that the for-loop body has the form:
var key;
var fallbackKey;
if (repository === "openSource") {
...
} else if (repository === "internal") {
...
} else {
key = repository;
fallbackKey = null;
}
this.revision[repository] = parseRevisionProperty(revisionProperty, key, fallbackKey);
<span class="quote">> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueue.js:269
> compareIterations: function(a, b)</span >
We should have a plan on how we are going to abstract this function to work on an arbitrary repository before landing this change.
<span class="quote">> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueue.js:271
> + var result = b.revision["openSource"] - a.revision["openSource"];</span >
I suggest that we create a named constant for the string literals "openSource" and referencing this constant throughout the patch instead of hardcoding "openSource" as a string literal. The benefit of this approach is that if we decide to change the repository name then we can update the the constant as opposed to performing a find-and-replace throughout the code for the string literal "openSource".
<span class="quote">> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueue.js:275
> + result = b.revision["internal"] - a.revision["internal"];</span >
Similarly, I suggest we create a named constant for the string literal "internal".
<span class="quote">> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueue.js:287
> compareIterationsByRevisions: function(a, b)</span >
We should have a plan on how we are going to abstract this function to work on an arbitrary repository before landing this change.
<span class="quote">> Tools/ChangeLog:18
> + Using "revision" property instead of "opensourceRevision" and "internalRevision".</span >
Minor: We tend to write the remark "Ditto." whenever the remark of the previous function change is applicable to the current function so as to avoid repeating the same remark.</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>