[webkit-reviews] review denied: [Bug 76801] Listboxes incorrectly display contents when cleared and then re-populated : [Attachment 124335] PatchWithTestCase

[bugzilla-daemon at webkit.org]

Simon Fraser (on vacation until 2-14) <simon.fraser at apple.com> has denied Joe
Thomas <joethomas at motorola.com>'s request for review:
Bug 76801: Listboxes incorrectly display contents when cleared and then
re-populated
https://bugs.webkit.org/show_bug.cgi?id=76801

Attachment 124335: PatchWithTestCase
https://bugs.webkit.org/attachment.cgi?id=124335&action=review

------- Additional Comments from Simon Fraser (on vacation until 2-14)
<simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=124335&action=review


You need pixel results for the new tests (run-webkit-tests --pixel). r- for
lack of pixel results.

> Source/WebCore/rendering/RenderListBox.cpp:258
> +	      ScrollableArea::scrollToYOffsetWithoutAnimation(0);

Does the call to scrollToYOffsetWithoutAnimation() have to be prefixed with the
class name? Are you specifically calling the base class implementation?

> LayoutTests/fast/forms/listbox-clear-restore-expected.html:10
> +    var myList = document.getElementById("myList"),
> +	   item,
> +	   ii;

We don't declare variables like this normally.

> LayoutTests/fast/forms/listbox-clear-restore-expected.html:12
> +    for (ii = 0; ii < 100; ii++) {

ii could be a loop variable for (var ii ...

> LayoutTests/fast/forms/listbox-clear-restore.html:12
> +    var myList = document.getElementById("myList"),
> +	   item,
> +	   ii;
> +
> +    for (ii = 0; ii < 100; ii++) {

Ditto.