Bug 147280 – Refactor to convert openSourceRevision and internalRevision properties on BuildbotIteration into a more generic collection of revisions

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Jason Marcell]

This purpose of this patch is to refactor the codebase in order to convert the openSourceRevision and internalRevision properties on BuildbotIteration into a more generic collection of revisions.

I've been working with Dan Bates on an iterative approach to refactoring the codebase so that we can ultimately handle different repositories other than "opensource" and "internal".

[Jason Marcell]

Created attachment 257486 [details]
Refactor to convert openSourceRevision and internalRevision properties on BuildbotIteration into a more generic collection of revisions.

[Daniel Bates]

Comment on attachment 257486 [details]
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.

[Jason Marcell]

Created attachment 257853 [details]
Refactor to convert openSourceRevision and internalRevision properties on BuildbotIteration into a more generic collection of revisions.

1. I fixed up the change log to use "Ditto."
2. I created a set of constants for repository names so that we're no longer using magic strings. I also have a plan in place for using these constants in a subsequent patch.
3. I moved the comment about "got_revision" formats to a more relevant section of the codebase.
4. I made the fallbackKey null by default.
5. I declared key and fallbackKey at the top of this if statement and created a new "else" clause:

var key;
var fallbackKey;
if (repository === "openSource") {
    ...
} else if (repository === "internal") {
    ...
} else {
    key = repository;
    fallbackKey = null;
}

[Daniel Bates]

Comment on attachment 257853 [details]
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=257853&action=review

Looks good. I noticed some very minor nits.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueueView.js:207
> +    _revisionContentWithPopoverForIteration: function(iteration, previousIteration, repository, trac)

Would repositoryName be a better name for the argument repository?

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueueView.js:265
> +        var openSourceContent = this._revisionContentWithPopoverForIteration(iteration, previousDisplayedIteration, "openSource", webkitTrac);

We should pass Dashboard.Repository.OpenSource.name to _revisionContentWithPopoverForIteration() instead of hardcoding the value of Dashboard.Repository.OpenSource.name.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueueView.js:270
> +        var internalContent = this._revisionContentWithPopoverForIteration(iteration, previousDisplayedIteration, "internal", internalTrac);

Similarly, we should pass Dashboard.Repository.Internal.name instead of hardcoding "internal" on this line.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Dashboard.js:41
> +        OpenSource: { name: "openSource", sort: 0 },
> +        Internal: { name: "internal", sort: 1 },

I suggest that we rename the property "sort" to "order" to be consistent with the name of the property with the same purpose in the Dashboard.Platform dictionaries.

[Jason Marcell]

Created attachment 258106 [details]
Refactor to convert openSourceRevision and internalRevision properties on BuildbotIteration into a more generic collection of revisions.

[Daniel Bates]

Comment on attachment 258106 [details]
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=258106&action=review

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:202
> +            if (repository === "openSource") {

We should use Dashboard.Repository.OpenSource.name instead of the string literal "openSource".

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:205
> +            } else if (repository === "internal") {

Similarly, we should use Dashboard.Repository.Internal.name instead of the string literal "internal".

> Tools/ChangeLog:11
> +        (parseRevisionProperty): Moved a comment up to parseRevisionProperty where it seems more relevent.

Nit: relevent => relevant

> Tools/ChangeLog:21
> +        Added "trac" parameter in order to specify the trac instance to use.

Nit: Please move this sentence such that it starts on line 20 (above) and break it across multiple lines if needed.

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

Nit: Please move this sentence such that it starts on the same line as the last sentence (above) and break it across multiple lines if needed.

> Tools/ChangeLog:23
> +        (BuildbotQueueView.prototype.revisionContentForIteration): Passing the "repository" and trac

Nit: the "repository" => the repository name

> Tools/ChangeLog:24
> +        instance to _revisionContentWithPopoverForIteration

Nit: _revisionContentWithPopoverForIteration => _revisionContentWithPopoverForIteration().
(Notice the period at the end of suggested substitution).

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

Nit: Please move this sentence such that it starts on line 24 (above) and break it across multiple lines if needed.

> Tools/ChangeLog:27
> +        constants for internal and opensource repositories

Nit: Missing a period at the end of this sentence.

[Jason Marcell]

Created attachment 258139 [details]
Refactor to convert openSourceRevision and internalRevision  properties on BuildbotIteration into a more generic collection of revisions.

[WebKit Commit Bot]

Comment on attachment 258139 [details]
Refactor to convert openSourceRevision and internalRevision  properties on BuildbotIteration into a more generic collection of revisions.

Clearing flags on attachment: 258139

Committed r187871: <http://trac.webkit.org/changeset/187871>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.