[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 #256746|review?                     |review-
              Flags|                            |

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

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

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotCombinedQueueView.js:74
> -            var status = new StatusLineView(message, StatusLineView.Status.Good, "all tests passed", null, null);
> +            var statusMessagePassed = "all " + (queue.builder ? "builds" : "tests") + " passed"
> +            var status = new StatusLineView(message, StatusLineView.Status.Good, statusMessagePassed, null, null);

I'm unclear how this change relates to the purpose of this bug. Although this change is straightforward, we may want to consider making this change in a separate bug/patch.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:202
>          // revision. Therefore, we only look at got_revision to extract the Internal revision when it's
>          // a dictionary.

The last paragraph of comment block above is out-of-date with the proposed code change. Either we should update the comment or remove the last paragraph from the comment block above.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:213
> +        for (project in this.queue.branch) {
> +            if (project === "openSource")
> +                this.openSourceRevision = parseInt(parseRevisionProperty(revisionProperty, "WebKit", "opensource"));
> +            else if (project === "internal")
> +                this.internalRevision = parseInt(parseRevisionProperty(revisionProperty, "Internal", "internal"));
> +            else
> +                this.otherRevision = parseRevisionProperty(revisionProperty, project, "Internal");

This does not seem correct. Assume this.queue.branch := {openSource: "trunk, internal: "trunk"} (which it is for all WebKit OpenSource Project queues by (*) and (**). Suppose revisionProperty = "2468". Then this.openSourceRevision, this.internalRevision = 2468. How does the dashboard behave?

(*) Tthere is no WebKit OpenSource Project queues that explicitly specify the property branch in <http://trac.webkit.org/browser/trunk/Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/WebKitBuildbot.js?rev=184879>.
(**) <http://trac.webkit.org/browser/trunk/Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Buildbot.js#L105>

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueueView.js:212
> +            // Using substr for Git Short-SHA

Either elaborate further on the non-obviousness of using String.substr() to truncate a SHA or remove this coment.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueueView.js:215
> +                content.textContent = content.textContent.substr(0, 8);

How did you choose 8? I mean, did you compute the collision probability as part of coming up with 8. Throughout <http://git-scm.com/book/ca/v1/Git-Tools-Revision-Selection#Short-SHA> and in various Git literature and tool output I see SHAs of length 7.

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

It is not clear here what the purpose of the boolean true is in the last argument. I suggest we define a local variable, say isInternalRevision, defined to be true and pass this named local variable instead of using the keyword true directly in the call to BuildbotQueueView.prototype._revisionContentWithPopoverForIteration().

> Tools/ChangeLog:3
> +        https://bugs.webkit.org/show_bug.cgi?id=146210

Missing bug title, which should be above this line.

-- 
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/20150716/4a337c10/attachment.html>