81848 – Show image diffs for gpu_tests on flakiness dashboard

Bug 81848 - Show image diffs for gpu_tests on flakiness dashboard

Summary: Show image diffs for gpu_tests on flakiness dashboard

Status:	RESOLVED FIXED

Alias:	None

Product:	WebKit
Classification:	Unclassified
Component:	New Bugs (show other bugs)
Version:	528+ (Nightly build)
Hardware:	Unspecified Unspecified

Importance:	P2 Normal
Assignee:	Nobody

URL:
Keywords:

Duplicates (1):	81847 (view as bug list)
Depends on:
Blocks:

Reported:	2012-03-21 17:18 PDT by Dave Tu
Modified:	2012-03-27 12:36 PDT (History)
CC List:	2 users (show)

See Also:

Attachments
Patch (9.72 KB, patch) 2012-03-21 17:21 PDT, Dave Tu	no flags	Details \| Formatted Diff \| Diff
Patch (9.72 KB, patch) 2012-03-21 17:28 PDT, Dave Tu	no flags	Details \| Formatted Diff \| Diff
Patch (10.42 KB, patch) 2012-03-26 18:24 PDT, Dave Tu	no flags	Details \| Formatted Diff \| Diff
Patch (10.40 KB, patch) 2012-03-27 11:46 PDT, Dave Tu	no flags	Details \| Formatted Diff \| Diff
Show Obsolete (3) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.

Description Dave Tu 2012-03-21 17:18:33 PDT

Show image diffs for gpu_tests on flakiness dashboard

Comment 1 Dave Tu 2012-03-21 17:19:52 PDT


*** This bug has been marked as a duplicate of bug 81847 ***

Comment 2 Dave Tu 2012-03-21 17:20:58 PDT

Reopening to attach new patch.

Comment 3 Dave Tu 2012-03-21 17:21:00 PDT

Created attachment 133151 [details]
Patch

Comment 4 Dave Tu 2012-03-21 17:21:20 PDT

*** Bug 81847 has been marked as a duplicate of this bug. ***

Comment 5 Dave Tu 2012-03-21 17:28:12 PDT

Created attachment 133153 [details]
Patch

Comment 6 Ojan Vafai 2012-03-21 19:07:09 PDT

Comment on attachment 133153 [details]
Patch

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.

Comment 7 Dave Tu 2012-03-26 18:24:01 PDT

Created attachment 133956 [details]
Patch

Comment 8 Dave Tu 2012-03-26 18:24:22 PDT

Comment on attachment 133153 [details]
Patch

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

>> ChangeLog:10
>> +        * Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:
> 
> This needs more description. The bug title only covers part of what this patch does.

Done.

>> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:1843
>> +        }
> 
> WebKit style is to not put the brackets around single line if/else.

Done.

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

Done.

>> 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. :)

Done.

>> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:-2156
>> -        container.parentNode.removeChild(childContainer);
> 
> Why did you remove this?

childContainer is not defined in this context, this doesn't do anything besides throw an error. Looks like it should be container.removeChild(dummyNode);

>> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:2184
>> +            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.

Done.

>> 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.

Done.

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

Done.

>> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:2190
>> +            title.innerText = opt_title;
> 
> Use textContent instead. innerText does weird things.

Done.

>> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:2192
>> +            childContainer.appendChild(document.createElement('br'));
> 
> Don't need this anymore now that title is a div.

Done.

>> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:2196
>> +            container.insertBefore(item, dummyNode);
> 
> How about calling replaceChild instead of insertBefore?

Done.

>> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:2197
>> +        }
> 
> Single-line else doesn't have brackets in webkit style.

Done.

Comment 9 Ojan Vafai 2012-03-27 11:11:02 PDT

Comment on attachment 133956 [details]
Patch

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

Please fix the nits and upload a new patch for the commit queue.

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

Single-line  else shouldn't have braces.

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

Single-line  else shouldn't have braces.

Comment 10 Dave Tu 2012-03-27 11:46:33 PDT

Created attachment 134108 [details]
Patch

Comment 11 WebKit Review Bot 2012-03-27 12:36:34 PDT

Comment on attachment 134108 [details]
Patch

Clearing flags on attachment: 134108

Committed r112303: <http://trac.webkit.org/changeset/112303>

Comment 12 WebKit Review Bot 2012-03-27 12:36:39 PDT

All reviewed patches have been landed.  Closing bug.