Bug 81848

Summary: Show image diffs for gpu_tests on flakiness dashboard
Product: WebKit Reporter: Dave Tu <dtu>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: ojan, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

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.