[Webkit-unassigned] [Bug 147280] Refactor to convert openSourceRevision and internalRevision properties on BuildbotIteration into a more generic collection of revisions

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=147280

Daniel Bates <dbates at webkit.org> changed:

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

--- Comment #2 from Daniel Bates <dbates at webkit.org> ---
Comment on attachment 257486
  --> https://bugs.webkit.org/attachment.cgi?id=257486
Refactor to convert openSourceRevision and internalRevision properties on BuildbotIteration into a more generic collection of revisions.

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

This patch looks good. I would like to see another iteration of it.

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

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.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:207
> +            var key = repository;
> +            var fallbackKey = "Internal";

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

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueue.js:269
>      compareIterations: function(a, b)

We should have a plan on how we are going to abstract this function to work on an arbitrary repository before landing this change.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueue.js:271
> +        var result = b.revision["openSource"] - a.revision["openSource"];

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

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueue.js:275
> +        result = b.revision["internal"] - a.revision["internal"];

Similarly, I suggest we create a named constant for the string literal "internal".

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueue.js:287
>      compareIterationsByRevisions: function(a, b)

We should have a plan on how we are going to abstract this function to work on an arbitrary repository before landing this change.

> Tools/ChangeLog:18
> +        Using "revision" property instead of "opensourceRevision" and "internalRevision".

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.

-- 
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/20150725/ebda30b0/attachment.html>