[Webkit-unassigned] [Bug 81848] Show image diffs for gpu_tests on flakiness dashboard

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=81848


Ojan Vafai <ojan at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #133153|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #6 from Ojan Vafai <ojan at chromium.org>  2012-03-21 19:07:09 PST ---
(From update of attachment 133153)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.