Refactor BuildbotQueueView._presentPopoverForPendingCommits to work more generically with repositories other than "openSource" and "internal".
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.