[webkit-reviews] review denied: [Bug 115734] Changing the selection pseudo style does not trigger a selection repaint : [Attachment 200925] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Sergio Villar Senin
<svillar at igalia.com>'s request for review:
Bug 115734: Changing the selection pseudo style does not trigger a selection
repaint
https://bugs.webkit.org/show_bug.cgi?id=115734

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

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=200925&action=review


No idea about correctness, but I think there are enough issues with the test to
have a second round:

> LayoutTests/ChangeLog:13
> +	   * fast/backgrounds/selection-change-style-expected.html: Added.
> +	   * fast/backgrounds/selection-change-style.html: Added.

Why is the test here and not in fast/repaint?

> LayoutTests/fast/backgrounds/selection-change-style-expected.html:4
> +<!--
> +This test checks that changing the selection style triggers a selection
redraw.
> +-->

In the expectations?

> LayoutTests/fast/backgrounds/selection-change-style.html:4
> +<!--
> +This test checks that changing the selection style triggers a selection
redraw.
> +-->

Since this test can be run manually, it would be better to have the text in the
test + an explanation of the expected results for someone trying the test in
their browser.

> LayoutTests/fast/backgrounds/selection-change-style.html:26
> +	   setTimeout(function() {redDiv.className =
"green";testRunner.notifyDone();}, 100);

Shouldn't this be using runRepaintTest?
Otherwise you will slow down testing quite a bit.