<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><span class="vcard"><a class="email" href="mailto:dbates@webkit.org" title="Daniel Bates <dbates@webkit.org>"> <span class="fn">Daniel Bates</span></a>
</span> changed
<a class="bz_bug_link
bz_status_NEW "
title="NEW - Allow other projects to display on the dashboard"
href="https://bugs.webkit.org/show_bug.cgi?id=146210">bug 146210</a>
<br>
<table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>What</th>
<th>Removed</th>
<th>Added</th>
</tr>
<tr>
<td style="text-align:right;">Attachment #256746 Flags</td>
<td>review?
</td>
<td>review-
</td>
</tr></table>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - Allow other projects to display on the dashboard"
href="https://bugs.webkit.org/show_bug.cgi?id=146210#c4">Comment # 4</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - Allow other projects to display on the dashboard"
href="https://bugs.webkit.org/show_bug.cgi?id=146210">bug 146210</a>
from <span class="vcard"><a class="email" href="mailto:dbates@webkit.org" title="Daniel Bates <dbates@webkit.org>"> <span class="fn">Daniel Bates</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=256746&action=diff" name="attach_256746" title="Patch to allow for projects other than open source and internal on the dashboard.">attachment 256746</a> <a href="attachment.cgi?id=256746&action=edit" title="Patch to allow for projects other than open source and internal on the dashboard.">[details]</a></span>
Patch to allow for projects other than open source and internal on the dashboard.
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=256746&action=review">https://bugs.webkit.org/attachment.cgi?id=256746&action=review</a>
<span class="quote">> 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);</span >
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.
<span class="quote">> 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.</span >
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.
<span class="quote">> 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");</span >
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 <<a href="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/WebKitBuildbot.js?rev=184879</a>>.
(**) <<a href="http://trac.webkit.org/browser/trunk/Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Buildbot.js#L105">http://trac.webkit.org/browser/trunk/Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Buildbot.js#L105</a>>
<span class="quote">> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueueView.js:212
> + // Using substr for Git Short-SHA</span >
Either elaborate further on the non-obviousness of using String.substr() to truncate a SHA or remove this coment.
<span class="quote">> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueueView.js:215
> + content.textContent = content.textContent.substr(0, 8);</span >
How did you choose 8? I mean, did you compute the collision probability as part of coming up with 8. Throughout <<a href="http://git-scm.com/book/ca/v1/Git-Tools-Revision-Selection#Short-SHA">http://git-scm.com/book/ca/v1/Git-Tools-Revision-Selection#Short-SHA</a>> and in various Git literature and tool output I see SHAs of length 7.
<span class="quote">> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueueView.js:271
> + return this._revisionContentWithPopoverForIteration(iteration, previousDisplayedIteration, true);</span >
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().
<span class="quote">> Tools/ChangeLog:3
> + <a class="bz_bug_link
bz_status_NEW "
title="NEW - Allow other projects to display on the dashboard"
href="show_bug.cgi?id=146210">https://bugs.webkit.org/show_bug.cgi?id=146210</a></span >
Missing bug title, which should be above this line.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>