On Mon, 31 Jul 2023 08:16:12 GMT, Jay Bhaskar <jbhas...@openjdk.org> wrote:
>> Issue: Some web worker tests fail to finish.
>> Root Cause:
>> sharedTimerFiredInternal function from ThreadTimers class does not require
>> an isMainThread check, The check was introduced during the initial webkit
>> stabilization.
>
> Jay Bhaskar has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Add test case for worker timeout
The fix and test both look good. I confirm that it fixes the problem and that
the new test fails without the fix and passes with the fix.
I left a couple minor comments on the test, but I'll approve it anyway. If you
choose to fix them, I'll reapprove.
modules/javafx.web/src/test/java/test/javafx/scene/web/WebWorkerTest.java line
50:
> 48: @After
> 49: public void after() {
> 50: }
Minor: you don't need this empty method.
modules/javafx.web/src/test/java/test/javafx/scene/web/WebWorkerTest.java line
64:
> 62: } catch (InterruptedException e) {
> 63: // Handle the exception if the thread is interrupted while
> sleeping
> 64: }
Minor: if you add `throws InterruptedException` to the test method you don't
need a try/catch here.
modules/javafx.web/src/test/java/test/javafx/scene/web/WebWorkerTest.java line
69:
> 67: WebView view = getView();
> 68: String res = (String)
> view.getEngine().executeScript("document.getElementById('result').innerText;");
> 69: assertEquals("4",res);
Minor: space after the `,`
-------------
Marked as reviewed by kcr (Lead).
PR Review: https://git.openjdk.org/jfx/pull/1191#pullrequestreview-1555894189
PR Review Comment: https://git.openjdk.org/jfx/pull/1191#discussion_r1279944478
PR Review Comment: https://git.openjdk.org/jfx/pull/1191#discussion_r1280000329
PR Review Comment: https://git.openjdk.org/jfx/pull/1191#discussion_r1280000445
