On Fri, 19 Jul 2024 13:53:08 GMT, Jay Bhaskar <jbhas...@openjdk.org> wrote:
>> Issue: CSSRounding.html DRT Test fails due sub pixel layout. The fractional
>> value adds up to 1px extra height.
>> Solution: The Prefer height calculation should do flooring of fractional
>> values. This is valid for Linear Layout which is a legacy support to most of
>> the browsers.
>
> Jay Bhaskar has updated the pull request incrementally with one additional
> commit since the last revision:
>
> restore master file
The fix looks good. So does the test (with minor comments). I confirm that the
test fails without the fix and passes with the fix.
I left mostly minor suggestions on the test, but will approve it as is. If you
choose to make the changes, I'll reapprove. Otherwise, please file a follow-up
bug to convert the test to JUnit 5; you can do the rest of the suggested
cleanup as part of that.
tests/system/src/test/java/test/javafx/scene/web/CSSRoundingTest.java line 41:
> 39: import org.junit.Before;
> 40: import org.junit.BeforeClass;
> 41: import org.junit.Test;
Since this is a new standalone test, it would be good to use JUnit 5 rather
than 4. This could be done as a follow-up if you prefer.
tests/system/src/test/java/test/javafx/scene/web/CSSRoundingTest.java line 84:
> 82: Scene scene = new Scene(webView, 150, 100);
> 83: cssRoundingTestApp.primaryStage.setScene(scene);
> 84: cssRoundingTestApp.primaryStage.show();
Suggestion: move all of this to the application `start()` method, then add the
following to trigger the `launchLatch`:
primaryStage.setOnShown(e -> Platform.runLater(launchLatch::countDown));
See [FullScreenExitTest.java : line
73](https://github.com/openjdk/jfx/blob/master/tests/system/src/test/java/test/javafx/stage/FullScreenExitTest.java#L73C1)
tests/system/src/test/java/test/javafx/scene/web/CSSRoundingTest.java line 88:
> 86: }
> 87:
> 88: @Test public void testCSSroundingForLinearLayout() {
Minor: move the `@Test` annotation to its own line.
tests/system/src/test/java/test/javafx/scene/web/CSSRoundingTest.java line 154:
> 152: assertTrue("Timeout when waiting for focus change ",
> Util.await(webViewStateLatch));
> 153: //introduce sleep , so that web contents would be loaded , then
> take snapshot for testing
> 154: Util.sleep(1000);
The sleep may or may not be needed, but OK to leave it. The comment is wrong,
though, since you aren't taking a snapshot.
tests/system/src/test/java/test/javafx/scene/web/CSSRoundingTest.java line 170:
> 168: assertEquals(31, topBottom);
> 169: assertEquals(31, bottomTop);
> 170:
Minor: you can remove the extra blank line here.
-------------
Marked as reviewed by kcr (Lead).
PR Review: https://git.openjdk.org/jfx/pull/1514#pullrequestreview-2188742017
PR Review Comment: https://git.openjdk.org/jfx/pull/1514#discussion_r1684676870
PR Review Comment: https://git.openjdk.org/jfx/pull/1514#discussion_r1684684385
PR Review Comment: https://git.openjdk.org/jfx/pull/1514#discussion_r1684685106
PR Review Comment: https://git.openjdk.org/jfx/pull/1514#discussion_r1684687295
PR Review Comment: https://git.openjdk.org/jfx/pull/1514#discussion_r1684688326
