Re: RFR: 8260163: IrresponsiveScriptTest.testInfiniteLoopInScript unit test fails on Windows [v2]
On Mon, 1 Feb 2021 16:52:10 GMT, Arun Joseph wrote: >> Windows uses `WorkQueueGeneric` instead of `WorkQueueWin` from WebKit 610.2 >> onwards. In `WorkQueueWin`, `WorkQueue::dispatchAfter()` had a 20 ms >> `slopAdjustment` as the timer (called from `::SetTimer`) sometimes fire a >> few ms early. The same is not present in `WorkQueueGeneric` and needs to be >> added. >> >> Also removing `WorkQueueWin` as it's removed for WebKit as well. > > Arun Joseph has updated the pull request incrementally with one additional > commit since the last revision: > > Update comment Looks good. - Marked as reviewed by kcr (Lead). PR: https://git.openjdk.java.net/jfx/pull/391
Re: RFR: 8260163: IrresponsiveScriptTest.testInfiniteLoopInScript unit test fails on Windows [v2]
On Mon, 1 Feb 2021 16:55:50 GMT, Kevin Rushforth wrote:
>> Arun Joseph has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Update comment
>
> modules/javafx.web/src/main/native/Source/WTF/wtf/generic/WorkQueueGeneric.cpp
> line 85:
>
>> 83: // so far.
>> 84: const Seconds slopAdjustment { 20_ms };
>> 85: if (delay)
>
> Since `delay` is an object of type `Seconds`, should this be `if
> (delay.milliseconds())`? Otherwise, won't it just check whether the object is
> non-null?
In
[Seconds.h](https://github.com/openjdk/jfx/blob/master/modules/javafx.web/src/main/native/Source/WTF/wtf/Seconds.h),
there's an `operator bool()` which checks the m_value.
-
PR: https://git.openjdk.java.net/jfx/pull/391
Re: RFR: 8260163: IrresponsiveScriptTest.testInfiniteLoopInScript unit test fails on Windows [v2]
On Mon, 1 Feb 2021 16:52:10 GMT, Arun Joseph wrote:
>> Windows uses `WorkQueueGeneric` instead of `WorkQueueWin` from WebKit 610.2
>> onwards. In `WorkQueueWin`, `WorkQueue::dispatchAfter()` had a 20 ms
>> `slopAdjustment` as the timer (called from `::SetTimer`) sometimes fire a
>> few ms early. The same is not present in `WorkQueueGeneric` and needs to be
>> added.
>>
>> Also removing `WorkQueueWin` as it's removed for WebKit as well.
>
> Arun Joseph has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Update comment
modules/javafx.web/src/main/native/Source/WTF/wtf/generic/WorkQueueGeneric.cpp
line 85:
> 83: // so far.
> 84: const Seconds slopAdjustment { 20_ms };
> 85: if (delay)
Since `delay` is an object of type `Seconds`, should this be `if
(delay.milliseconds())`? Otherwise, won't it just check whether the object is
non-null?
-
PR: https://git.openjdk.java.net/jfx/pull/391
Re: RFR: 8260163: IrresponsiveScriptTest.testInfiniteLoopInScript unit test fails on Windows [v2]
> Windows uses `WorkQueueGeneric` instead of `WorkQueueWin` from WebKit 610.2 > onwards. In `WorkQueueWin`, `WorkQueue::dispatchAfter()` had a 20 ms > `slopAdjustment` as the timer (called from `::SetTimer`) sometimes fire a few > ms early. The same is not present in `WorkQueueGeneric` and needs to be added. > > Also removing `WorkQueueWin` as it's removed for WebKit as well. Arun Joseph has updated the pull request incrementally with one additional commit since the last revision: Update comment - Changes: - all: https://git.openjdk.java.net/jfx/pull/391/files - new: https://git.openjdk.java.net/jfx/pull/391/files/739f3207..ea61414f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=391=01 - incr: https://webrevs.openjdk.java.net/?repo=jfx=391=00-01 Stats: 10 lines in 1 file changed: 8 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jfx/pull/391.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/391/head:pull/391 PR: https://git.openjdk.java.net/jfx/pull/391
Re: RFR: 8260163: IrresponsiveScriptTest.testInfiniteLoopInScript unit test fails on Windows
On Mon, 1 Feb 2021 03:31:10 GMT, Arun Joseph wrote:
> Windows uses `WorkQueueGeneric` instead of `WorkQueueWin` from WebKit 610.2
> onwards. In `WorkQueueWin`, `WorkQueue::dispatchAfter()` had a 20 ms
> `slopAdjustment` as the timer (called from `::SetTimer`) sometimes fire a few
> ms early. The same is not present in `WorkQueueGeneric` and needs to be added.
>
> Also removing `WorkQueueWin` as it's removed for WebKit as well.
The fix and the test look good with one question and one doc comment.
modules/javafx.web/src/main/native/Source/WTF/wtf/generic/WorkQueueGeneric.cpp
line 78:
> 76: // Adding the slop adjustment from wtf/win/WorkQueueWin.cpp
> 77: const Seconds slopAdjustment { 20_ms };
> 78: delay += slopAdjustment;
Can you replace the above comment with the actual comment from the
(now-deleted) `WorkQueueWin.cpp` file? Also, I see that the original fix only
added the `slopAdjustment` if `delay` was non-zero. Should that be preserved?
-
PR: https://git.openjdk.java.net/jfx/pull/391
RFR: 8260163: IrresponsiveScriptTest.testInfiniteLoopInScript unit test fails on Windows
Windows uses `WorkQueueGeneric` instead of `WorkQueueWin` from WebKit 610.2 onwards. In `WorkQueueWin`, `WorkQueue::dispatchAfter()` had a 20 ms `slopAdjustment` as the timer (called from `::SetTimer`) sometimes fire a few ms early. The same is not present in `WorkQueueGeneric` and needs to be added. Also removing `WorkQueueWin` as it's removed for WebKit as well. - Commit messages: - 8260163: IrresponsiveScriptTest.testInfiniteLoopInScript unit test fails on Windows Changes: https://git.openjdk.java.net/jfx/pull/391/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx=391=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8260163 Stats: 204 lines in 3 files changed: 5 ins; 199 del; 0 mod Patch: https://git.openjdk.java.net/jfx/pull/391.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/391/head:pull/391 PR: https://git.openjdk.java.net/jfx/pull/391
