[webkit-reviews] review denied: [Bug 81848] Show image diffs for gpu_tests on flakiness dashboard : [Attachment 133153] Patch

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has denied Dave Tu <dtu at chromium.org>'s request
for review:
Bug 81848: Show image diffs for gpu_tests on flakiness dashboard
https://bugs.webkit.org/show_bug.cgi?id=81848

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

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=133153&action=review


Mostly just a bunch of nits...

> ChangeLog:10
> +
> +	   * Tools/TestResultServer/static-dashboards/builders.js:
> +	   * Tools/TestResultServer/static-dashboards/dashboard_base.js:
> +	   * Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:


This needs more description. The bug title only covers part of what this patch
does.

> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:1843
> +	   } else {
> +	       html += ' | <b>Only shows actual results/diffs from the most
recent *failure* on each bot.</b>';
> +	   }

WebKit style is to not put the brackets around single line if/else.

> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:2140
> +    var failure = indexesForFailures(builder, test)[0];

s/failure/failureIndex

> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:2158
> +    var failures = indexesForFailures(builder, test);

s/failures/failureIndexes. The old name of failures should have been
buildNumbers. :)

> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:-2156
> -    dummyNode.onerror = function() {
> -	   container.parentNode.removeChild(childContainer);

Why did you remove this?

> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:2184
> +	   if (opt_width > 0)
> +	       item.style.width = opt_width + 'px';

I'd rather you set a classname and set these widths in CSS. I'm not sure why we
explicitly set the height on line 2182. Looks like it's configurable.

So, you could make itemClassName the third argument to appendNonWebKitResults
and not optional.

> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:2187
> +	       var childContainer = document.createElement('span');

It would be better to make this a div with display:inline-block.

> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:2189
> +	       var title = document.createElement('b');

Make this a div.

> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:2190
> +	       title.innerText = opt_title;

Use textContent instead. innerText does weird things.

> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:2192
> +	       childContainer.appendChild(document.createElement('br'));

Don't need this anymore now that title is a div.

> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:2196
> +	       container.insertBefore(childContainer, dummyNode);
> +	   } else {
> +	       container.insertBefore(item, dummyNode);

How about calling replaceChild instead of insertBefore?

> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:2197
> +	   }

Single-line else doesn't have brackets in webkit style.