[Webkit-unassigned] [Bug 146210] Allow other projects to display on the dashboard

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=146210

Daniel Bates <dbates at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #255728|review?                     |review-
              Flags|                            |

--- Comment #2 from Daniel Bates <dbates at webkit.org> ---
Comment on attachment 255728
  --> https://bugs.webkit.org/attachment.cgi?id=255728
Patch to allow for projects other than open source and internal on the dashboard.

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

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:116
> +    if (isGitGotRevisonProperty(property, value))
> +        return value;
> +    else
> +        return parseInt(value);

We should have this function return a string and make it the caller's responsibility to parse the string for a number if needed. Then we can remove the function isGitGotRevisonProperty().

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:212
> +        for (project in this.queue.branch) {

This code is assuming that this.queue.branch represents a dictionary of (project, branch name)-pairs. Currently, this.queue.branch represents a dictionary of (repository, branch name)-pairs. Is this acceptable?

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:215
> +            var revisionProperty = data.properties.findFirst(function(property) {
> +                return isMultiCodebaseGotRevisionProperty(property) || property[0] === "got_revision";
> +            });

Notice that we compute this on each iteraiton. I suggest we move this out of the loop.

Also, the first disjunct is unnecessary by definition of isMultiCodebaseGotRevisionProperty().

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:222
> +                this.otherRevision = parseRevisionProperty(revisionProperty, project, "Tools");

Is it correct to fallback to using key Tools?

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueueView.js:211
> +        if (iteration.otherRevision)
> +            content.textContent = iteration.otherRevision.substring(0, 8);

This code assumes that iteration.otherRevision is always a Git revision. Is this correct?

How did you choose 8?

Did you mean to use String.substring() which takes a from and to argument for the indices?

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueueView.js:267
> +            return this._revisionContentWithPopoverForIteration(iteration, previousDisplayedIteration, true);

How does this work if previousDisplayedIteration is non-null?

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20150630/42750d35/attachment.html>