[webkit-reviews] review granted: [Bug 175593] undefined URL in PopoverTracker for failed step : [Attachment 318167] Updated patch

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has granted Aakash Jain
<aakash_jain at apple.com>'s request for review:
Bug 175593: undefined URL in PopoverTracker for failed step
https://bugs.webkit.org/show_bug.cgi?id=175593

Attachment 318167: Updated patch

https://bugs.webkit.org/attachment.cgi?id=318167&action=review




--- Comment #3 from Daniel Bates <dbates at webkit.org> ---
Comment on attachment 318167
  --> https://bugs.webkit.org/attachment.cgi?id=318167
Updated patch

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

>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/B
uildbotIteration.js:316
> +    _urlForStep: function(step)

Maybe a better name for this would be stdioURLForStep as this function returns
the URL to the stdio page for the step.

>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/B
uildbotIteration.js:326
> +	   return this.queue.buildbot.buildPageURLForIteration(this) +
"/steps/" + step.number + "/logs/stdio";

I suggest we add a FIXME comment above this line to explain that Buildbot 0.9
should provide this URL in the JSON output for the step as it does in Buildbot
versions less than 0.9. Even better we should file a Buildbot bug (if one does
not already exist) and reference it in this comment.

>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/B
uildbotTesterQueueView.js:115
> +			   var url = failedStep.URL ? failedStep.URL :
iteration.queue.buildbot.buildPageURLForIteration(iteration);

I suggest we revert this change.

>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/B
uildbotTesterQueueView.js:265
> +	       else {
> +		   var url = failedStep.URL ? failedStep.URL :
iteration.queue.buildbot.buildPageURLForIteration(iteration);
> +		  
addResultKind(this._testStepFailureDescriptionWithCount(failedStep), url);
> +	       }
>	   }, this);

Ditto.