On Wed, 10 Jul 2024 14:36:51 GMT, Andy Goryachev <ango...@openjdk.org> wrote:
>> Marius Hanl has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains 12 additional >> commits since the last revision: >> >> - Merge branch 'master' of https://github.com/openjdk/jfx into >> 8296387-tooltip-css >> - Use Base64 encoder in tests >> - Improve time measurement and simplify test diff code >> - add many more unit tests for Tooltip >> - Use Helper class instead >> - Doc >> - Add a test for changing the stylesheet and always process CSS for that >> matter >> - Add more documentation and improve css stylesheet test threshold >> - Implement applyStylesheetFromOwner(..) and use it instead to ensure >> correct CSS processing for the Tooltip Node. >> - Merge branch 'master' of https://github.com/openjdk/jfx into >> 8296387-tooltip-css >> - ... and 2 more: https://git.openjdk.org/jfx/compare/5c7c8bae...f917d18e > > modules/javafx.controls/src/test/java/test/javafx/scene/control/TooltipTest.java > line 709: > >> 707: } >> 708: >> 709: private String toBase64(String css) { > > suggestion: > > private static String encode(String css) { > return "data:base64," + > Base64.getUrlEncoder().encodeToString(css.getBytes(StandardCharsets.UTF_8)); > } I agree that it could be static, although I prefer the name `toBase64`. > tests/system/src/test/java/test/robot/javafx/scene/TooltipTest.java line 233: > >> 231: window.getX() + scene.getX() + button.getLayoutX() >> + button.getLayoutBounds().getWidth() / 2, >> 232: window.getY() + scene.getY() + button.getLayoutY() >> + button.getLayoutBounds().getHeight() / 2); >> 233: tooltipStartTime = System.currentTimeMillis(); > > it is better to use `System.nanoTime()` instead of `currentTimeMillis()` for > this kind of measurements because it is not affected by the updates to the > current time (such as time sync). > > (since there are plenty of other places where we use `currentTimeMillis()` > it's probably ok - very unlikely for the test to hit this scenario) That seems like a good overall test cleanup that could be done in a follow-up issue. ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/1394#discussion_r1672460851 PR Review Comment: https://git.openjdk.org/jfx/pull/1394#discussion_r1672494529
