[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
bugzilla-daemon at webkit.org
Tue May 7 20:56:08 PDT 2013
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.
More information about the webkit-reviews
mailing list