[webkit-reviews] review granted: [Bug 66955] garden-o-matic needs a way to report where and how tests are failing in the summary view. : [Attachment 105213] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted Dimitri Glazkov (Google)
<dglazkov at chromium.org>'s request for review:
Bug 66955: garden-o-matic needs a way to report where and how tests are failing
in the summary view.
https://bugs.webkit.org/show_bug.cgi?id=66955

Attachment 105213: Patch
https://bugs.webkit.org/attachment.cgi?id=105213&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=105213&action=review


>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/script
s/ui/failures.js:63
> +	   var titles = this.createTHead().insertRow();

Woah!  I never new about this API.

>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/script
s/ui/failures.js:76
> +	   row = this._resultRows[result] = this._body.insertRow(-1);

I don't use this pattern in C++ in WebKit, but it's probably fine to use here.

>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/script
s/ui/failures.js:87
> +	       this._resultRows = {};

Do you mean to blow away the entire dictionary for each key in the dictionary? 
That seems slightly wrong.

>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/script
s/ui/failures.js:95
> +	       var cell = row.cells[configuration.debug ? 2 : 1];

I would have put "configuration.debug ? 2 : 1" into a function just to give the
operation a name.

>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/script
s/ui/notifications_unittests.js:117
> +	      
'<thead><tr><td>type</td><td>release</td><td>debug</td></tr></thead>' +
> +	       '<tbody></tbody>' +

Should one of theres tests put something non-trivial into the tbody?