Bug 147805 – Refactor webkitTrac to Dashboard.Repository.OpenSource.trac and internalTrac to Dashboard.Repository.Internal.trac

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

[Jason Marcell]

Refactor webkitTrac to Dashboard.Repository.OpenSource.trac and internalTrac to Dashboard.Repository.Internal.trac

[Jason Marcell]

Created attachment 258561 [details]
Patch

[WebKit Commit Bot]

Attachment 258561 [details] did not pass style-queue:


ERROR: Tools/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jason Marcell]

Created attachment 258562 [details]
Patch

[Daniel Bates]

Comment on attachment 258562 [details]
Patch

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

Note, we'll need to make a corresponding change to our internal code before/after we land this change.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueueView.js:97
> -        var latestRecordedOpenSourceRevisionNumber = webkitTrac.latestRecordedRevisionNumber;
> -        if (!latestRecordedOpenSourceRevisionNumber || webkitTrac.oldestRecordedRevisionNumber > latestProductiveIteration.revision[Dashboard.Repository.OpenSource.name]) {
> -            webkitTrac.loadMoreHistoricalData();
> +        var latestRecordedOpenSourceRevisionNumber = Dashboard.Repository.OpenSource.trac.latestRecordedRevisionNumber;
> +        if (!latestRecordedOpenSourceRevisionNumber || Dashboard.Repository.OpenSource.trac.oldestRecordedRevisionNumber > latestProductiveIteration.revision[Dashboard.Repository.OpenSource.name]) {
> +            Dashboard.Repository.OpenSource.trac.loadMoreHistoricalData();
>              return;
>          }
>  
>          // FIXME: To be 100% correct, we should also filter out changes that are ignored by
>          // the queue, see _should_file_trigger_build in wkbuild.py.
> -        var totalRevisionsBehind = webkitTrac.commitsOnBranch(queue.branch.openSource, function(commit) { return commit.revisionNumber > latestProductiveIteration.revision[Dashboard.Repository.OpenSource.name]; }).length;
> +        var totalRevisionsBehind = Dashboard.Repository.OpenSource.trac.commitsOnBranch(queue.branch.openSource, function(commit) { return commit.revisionNumber > latestProductiveIteration.revision[Dashboard.Repository.OpenSource.name]; }).length;
>  
>          if (latestProductiveIteration.revision[Dashboard.Repository.Internal.name]) {
> -            var latestRecordedInternalRevisionNumber = internalTrac.latestRecordedRevisionNumber;
> -            if (!latestRecordedInternalRevisionNumber || internalTrac.oldestRecordedRevisionNumber > latestProductiveIteration.revision[Dashboard.Repository.Internal.name]) {
> -                internalTrac.loadMoreHistoricalData();
> +            var latestRecordedInternalRevisionNumber = Dashboard.Repository.Internal.trac.latestRecordedRevisionNumber;
> +            if (!latestRecordedInternalRevisionNumber || Dashboard.Repository.Internal.trac.oldestRecordedRevisionNumber > latestProductiveIteration.revision[Dashboard.Repository.Internal.name]) {
> +                Dashboard.Repository.Internal.trac.loadMoreHistoricalData();
>                  return;
>              }
>  
> -            totalRevisionsBehind += internalTrac.commitsOnBranch(queue.branch.internal, function(commit) { return commit.revisionNumber > latestProductiveIteration.revision[Dashboard.Repository.Internal.name]; }).length;
> +            totalRevisionsBehind += Dashboard.Repository.Internal.trac.commitsOnBranch(queue.branch.internal, function(commit) { return commit.revisionNumber > latestProductiveIteration.revision[Dashboard.Repository.Internal.name]; }).length;
>          }

For your consideration, I suggest we define local variables webkitTrac :=  Dashboard.Repository.OpenSource.trac and internalTrac := Dashboard.Repository.Internal.trac. Then we do no need to modify all the existing lines in this function. Additionally, defining such local variables will likely make it more straightforward to refactor this code to work for arbitrary repositories in a future patch among other benefits.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueueView.js:162
> -        var linesForOpenSource = this._popoverLinesForCommitRange(webkitTrac, queue.branch.openSource, latestProductiveIteration.revision[Dashboard.Repository.OpenSource.name] + 1, webkitTrac.latestRecordedRevisionNumber);
> +        var linesForOpenSource = this._popoverLinesForCommitRange(Dashboard.Repository.OpenSource.trac, queue.branch.openSource, latestProductiveIteration.revision[Dashboard.Repository.OpenSource.name] + 1, Dashboard.Repository.OpenSource.trac.latestRecordedRevisionNumber);
>          for (var i = 0; i != linesForOpenSource.length; ++i)
>              content.appendChild(linesForOpenSource[i]);
>  
>          var linesForInternal = [];
> -        if (latestProductiveIteration.revision[Dashboard.Repository.Internal.name] && internalTrac.latestRecordedRevisionNumber)
> -            var linesForInternal = this._popoverLinesForCommitRange(internalTrac, queue.branch.internal, latestProductiveIteration.revision[Dashboard.Repository.Internal.name] + 1, internalTrac.latestRecordedRevisionNumber);
> +        if (latestProductiveIteration.revision[Dashboard.Repository.Internal.name] && Dashboard.Repository.Internal.trac.latestRecordedRevisionNumber)
> +            var linesForInternal = this._popoverLinesForCommitRange(Dashboard.Repository.Internal.trac, queue.branch.internal, latestProductiveIteration.revision[Dashboard.Repository.Internal.name] + 1, Dashboard.Repository.Internal.trac.latestRecordedRevisionNumber);

Ditto.

[Daniel Bates]

Comment on attachment 258562 [details]
Patch

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

> Tools/ChangeLog:10
> +        (BuildbotQueueView): Moved webkitTrac to Dashboard.Repository.OpenSource.trac and internalTrac to
> +        Dashboard.Repository.Internal.trac

Very minor: "moved" is not the best way to describe the substitution you are performing. Also, we should add a period at the end of the sentence.

[Jason Marcell]

Created attachment 258759 [details]
Patch

[Daniel Bates]

Comment on attachment 258759 [details]
Patch

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

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueueView.js:80
> +        webkitTrac = Dashboard.Repository.OpenSource.trac;
> +        internalTrac = Dashboard.Repository.Internal.trac;

These lines are declaring global variables named webkitTrac and internalTrac. Please make these local variables by explicitly adding the keyword var before the name of the variable such that these lines read:

var webkitTrac = Dashboard.Repository.OpenSource.trac;
var internalTrac = Dashboard.Repository.Internal.trac;

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueueView.js:159
> +        webkitTrac = Dashboard.Repository.OpenSource.trac;
> +        internalTrac = Dashboard.Repository.Internal.trac;

Ditto.

[Jason Marcell]

Created attachment 258762 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 258762 [details]
Patch

Clearing flags on attachment: 258762

Committed r188304: <http://trac.webkit.org/changeset/188304>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.