RFR: 8336798: DRT test cssrounding.html test for linear layout fails with WebKit 619.1 [v3]

[Kevin Rushforth]

On Fri, 19 Jul 2024 13:53:08 GMT, Jay Bhaskar <jbhaskar at 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