Summary: | Refactor BuildbotQueueView._presentPopoverForPendingCommits to work more generically with repositories other than "openSource" and "internal". | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jason Marcell <jmarcell> | ||||||||||
Component: | Tools / Tests | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | adele, ap, bburg, commit-queue, dbates, jmarcell | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Jason Marcell
2015-08-12 17:30:56 PDT
Created attachment 258859 [details]
Patch
Comment on attachment 258859 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=258859&action=review > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueueView.js:172 > + if (shouldAddDivider) > + this._addDividerToPopover(content); > > - if (linesForOpenSource.length && linesForInternal.length) > - this._addDividerToPopover(content); > + for (var i = 0; i != lines.length; ++i) > + content.appendChild(lines[i]); > This is not correct because we always add a divider regardless of whether there were popover lines. (In reply to comment #2) > This is not correct because we always add a divider regardless of whether > there were popover lines. I meant to write: This is not correct because we always add a divider regardless of whether there were popover lines when shouldAddDivider is true. I see. I think we want that to say: if (lines.length) shouldAddDivider = true; Right? Created attachment 258946 [details]
Patch
Comment on attachment 258946 [details]
Patch
Now checking lines.length before adding the divider.
Comment on attachment 258946 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=258946&action=review > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueueView.js:174 > + if (lines.length) > + shouldAddDivider = true; This is not correct. Suppose there three repositories A, B, C and when processing these repositories (in this order) lines.length = 1, 0, 1, respectively. Then the output content will look like A_1, "divider", "divider", C_1. (In reply to comment #7) > [...] > Then the output content will look like A_1, "divider", "divider", C_1. I should add that we want the output content to look like: A_1, "divider", C_1 Created attachment 259046 [details]
Patch
I reset the shouldAddDivider variable after adding a divider. I also moved the lines variable assignment closer to it's usage. Comment on attachment 259046 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=259046&action=review > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueueView.js:177 > + var shouldAddDivider = false; > + var sortedRepositories = Dashboard.sortedRepositories; > + for (var i = 0; i < sortedRepositories.length; i++) { > + var repository = sortedRepositories[i]; > + var trac = repository.trac; > + var repositoryName = repository.name; > + var latestProductiveRevisionNumber = latestProductiveIteration.revision[repositoryName]; > + var lines = []; > + if (latestProductiveRevisionNumber && trac.latestRecordedRevisionNumber) { > + if (shouldAddDivider) { > + this._addDividerToPopover(content); > + shouldAddDivider = false; > + } > + > + lines = this._popoverLinesForCommitRange(trac, queue.branch[repositoryName], latestProductiveRevisionNumber + 1, trac.latestRecordedRevisionNumber); > + for (var i = 0; i != lines.length; ++i) > + content.appendChild(lines[i]); > + > + if (lines.length) > + shouldAddDivider = true; > + } > + } This is not correct. Suppose there are three repositories A, B and when processing these repositories (in this order) lines.length = 1, 0, respectively. Then the output content will look like A_1, "divider". But it should be A_1. Maybe something like this will work: var shouldAddDivider = false; var sortedRepositories = Dashboard.sortedRepositories; for (var i = 0; i < sortedRepositories.length; ++i) { var repository = sortedRepositories[i]; var trac = repository.trac; var repositoryName = repository.name; var latestProductiveRevisionNumber = latestProductiveIteration.revision[repositoryName]; if (!latestProductiveRevisionNumber || !trac.latestRecordedRevisionNumber) continue; var lines = this._popoverLinesForCommitRange(trac, queue.branch[repositoryName], latestProductiveRevisionNumber + 1, trac.latestRecordedRevisionNumber); var length = lines.length; if (length && shouldAddDivider) this._addDividerToPopover(content); for (var i = 0; i < length; ++i) content.appendChild(lines[i]); shouldAddDivider = shouldAddDivider || length > 0; } Created attachment 259161 [details]
Patch
Comment on attachment 259161 [details]
Patch
Thanks, Dan. I looked over your suggested change over the weekend. It looks correct and also very concise.
Comment on attachment 259161 [details] Patch Clearing flags on attachment: 259161 Committed r188533: <http://trac.webkit.org/changeset/188533> All reviewed patches have been landed. Closing bug. |