[Webkit-unassigned] [Bug 81848] Show image diffs for gpu_tests on flakiness dashboard
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Mar 26 18:24:22 PDT 2012
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.
More information about the webkit-unassigned
mailing list