RFR: 8296387: [Tooltip, CSS] -fx-show-delay is only applied to the first tooltip that is shown before it is displayed [v10]

[Kevin Rushforth]

On Wed, 10 Jul 2024 14:36:51 GMT, Andy Goryachev <angorya at 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