[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





--- Comment #8 from Dave Tu <dtu at chromium.org>  2012-03-26 18:24:22 PST ---
(From update of attachment 133153)
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.

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