Bug 147796 – Refactor BuildbotQueueView.revisionContentForIteration to work more generically with repositories other than "openSource" and "internal".

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

[Jason Marcell]

Refactor BuildbotQueueView.revisionContentForIteration to work more generically with repositories other than "openSource" and "internal".

[Jason Marcell]

Created attachment 258537 [details]
Patch

[Daniel Bates]

Comment on attachment 258537 [details]
Patch

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

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueueView.js:276
> +        var shouldAddPlusSign = false;
> +        var sortedRepositories = Dashboard.sortedRepositories;
> +        for (var i = 0; i < sortedRepositories.length; ++i) {
> +            var repository = sortedRepositories[i];
> +            var content = this._revisionContentWithPopoverForIteration(iteration,
> +                previousDisplayedIteration, repository);
> +            if (content) {
> +                if (shouldAddPlusSign)
> +                    fragment.appendChild(document.createTextNode(" \uff0b "));
> +                fragment.appendChild(content);
> +                shouldAddPlusSign = true;
> +            }
> +        }

This change does not look correct. We are neither passing an instance of the Trac object to _revisionContentWithPopoverForIteration() nor are we passing the name of the repository to _revisionContentWithPopoverForIteration().

Assuming we store a reference to the Trac object in the repository object such that you can access it via sortedRepositories[i].trac for some i in [0, Dashboard.sortedRepositories.length], then I would write this as:

var sortedRepositories = Dashboard.sortedRepositories;
var fragment = document.createDocumentFragment();
fragment.appendChild(this._revisionContentWithPopoverForIteration(iteration, previousDisplayedIteration, sortedRepositories[0].name, sortedRepositories[0].trac));
for (var i = 1; i < sortedRepositories.length; ++i) {
    fragment.appendChild(document.createTextNode(" \uff0b "));
    fragment.appendChild(this._revisionContentWithPopoverForIteration(iteration, previousDisplayedIteration, sortedRepositories[i].name, sortedRepositories[0].trac));
}
console.assert(fragment.childNodes.length);

[Daniel Bates]

(In reply to comment #2)
> [...]
> Dashboard.sortedRepositories.length], then I would write this as:
> 
> var sortedRepositories = Dashboard.sortedRepositories;
> var fragment = document.createDocumentFragment();
> fragment.appendChild(this._revisionContentWithPopoverForIteration(iteration,
> previousDisplayedIteration, sortedRepositories[0].name,
> sortedRepositories[0].trac));
> for (var i = 1; i < sortedRepositories.length; ++i) {
>     fragment.appendChild(document.createTextNode(" \uff0b "));
>     fragment.appendChild(this._revisionContentWithPopoverForIteration(iteration, previousDisplayedIteration, sortedRepositories[i].name, sortedRepositories[0].trac));

err, the for-loop body should read:

var content = this._revisionContentWithPopoverForIteration(iteration, previousDisplayedIteration, sortedRepositories[i].name, sortedRepositories[0].trac);

if (!content)
    continue;

fragment.appendChild(document.createTextNode(" \uff0b ")); 
fragment.appendChild(content);

[Jason Marcell]

Created attachment 258771 [details]
Patch

[Jason Marcell]

Created attachment 258773 [details]
Patch

[Jason Marcell]

Comment on attachment 258773 [details]
Patch

Modified the body of the for loop per Daniel Bates' suggestion.

Also added changes to _revisionContentWithPopoverForIteration.

[Daniel Bates]

Comment on attachment 258773 [details]
Patch

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

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueueView.js:214
> +        if (!iteration.revision[repository.name])
> +            return null;

We should assert this invariant instead of returning null as it represents a programmer mistake.

console.assert(iteration.revision[repository.name]);

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueueView.js:274
> +            var content = this._revisionContentWithPopoverForIteration(iteration,
> +                previousDisplayedIteration, sortedRepositories[i]);

Please write this on one line.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueueView.js:282
>          return fragment;

We may want to consider asserting that the fragment has at least one child node:

console.assert(fragment.childNodes.length);

[Jason Marcell]

Created attachment 258824 [details]
Patch

[Jason Marcell]

Comment on attachment 258824 [details]
Patch

Amended patch with suggestions from Daniel Bates.

[Daniel Bates]

Comment on attachment 258824 [details]
Patch

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

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueueView.js:213
> +        console.assert(iteration.revision[repository.name]);

We should also assert that previousIteration.revision[repository.name] is non-null..

Also, I suggest that we cache the repository name (repository.name) in a local variable, say repositoryName, and reference this variable throughout this function instead of accessing repository.name repeatedly.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueueView.js:272
> +            if (!iteration.revision[sortedRepositories[i].name])

For your consideration, I suggest defining a local variable, say repository, equal to sortedRepositories[i] and then referencing this local variable on this line and line 274 so that we do not compute sortedRepositories[i] twice.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueueView.js:276
> +            if (!content)
> +                continue;

This if-statement is unnecessary given that we ensure the invariant of _revisionContentWithPopoverForIteration() on line 272.

[Jason Marcell]

Created attachment 258830 [details]
Patch

[Jason Marcell]

Comment on attachment 258830 [details]
Patch

At Daniel Bates' recommendation, added additional assertion that, if the previousIteration is non-null, the given repository is in previousIteration.revision.

Also at Daniel's recommendation, defined a local variable for repositoryName and removed an unnecessary if statement.

[Daniel Bates]

Comment on attachment 258830 [details]
Patch

Thank you for iterating on the patch, Jason.

r=me

[WebKit Commit Bot]

Comment on attachment 258830 [details]
Patch

Clearing flags on attachment: 258830

Committed r188359: <http://trac.webkit.org/changeset/188359>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.