Re: RFR: [WIP] 8088739: [TestBug] RegionBackgroundImageUITest fail with timeout error [v2]
On Mon, 12 Apr 2021 14:33:08 GMT, Ambarish Rapte wrote: >> Each test in RegionBackgroundImageUITest makes several calls to >> `robot.getPixelColor()` on App thread. Due to this each test requires more >> than **60** seconds for execution. >> >> Fix is to save a screen capture of Scene (on App thread) and then read pixel >> color from the screen capture(not on app thread). This reduces execution >> time of each test to less than **3** seconds. >> Ideally with this fix(commit#1) all the tests should pass. All tests do pass >> on Windows and Linux but three tests fail on Mac, which used to pass without >> this change. >> - RegionBackgroundImageUITest.unalignedImage >> - RegionBackgroundFillUITest.testScenario2 >> - RegionBackgroundFillUITest.testScenario3 >> >> commit#2 solves the above problem. Solution is to fallback to test color >> again by reading it using `robot.getPixelColor()` on App thread when a test >> fails. >> >> One test RegionBackgroundImageUITest.unalignedImage_Cover, fails only on Mac >> platform, before and after this fix. >> It is reported as a new issue >> [JDK-8255679](https://bugs.openjdk.java.net/browse/JDK-8255679) >> >> This is a test fix and affects only the tests that extend from >> `RegionUITestBase` test class and does not affect other tests. >> Verified that `RegionBackgroundImageUITest` and `RegionBackgroundFillUITest` >> tests pass on all three platforms(except >> RegionBackgroundImageUITest.unalignedImage_Cover which fails on Mac). > > Ambarish Rapte 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 three additional > commits since the last revision: > > - Merge > - fix for tests failing on Mac > - ideal fix, but causes 3 test fail Following tests fail on windows with non-integer screen scale. **30** tests fail with **125%** scale, alignedImage alignedImage_Contain alignedImage_PositionBottomRight alignedImage_PositionBottomRightRepeatX alignedImage_PositionBottomRightRepeatY alignedImage_PositionCenterBottom alignedImage_PositionCenterBottomRepeatX alignedImage_PositionCenterBottomRepeatY alignedImage_PositionCenterFiftyPercentRepeatY alignedImage_PositionCenterLeftRepeatY alignedImage_PositionCenterRepeatY alignedImage_PositionCenterRightRepeatY alignedImage_PositionCenterTopRepeatY alignedImage_RepeatY alignedImage_Round alignedImage_RoundSpace alignedImage_Space unalignedImage unalignedImage_Contain unalignedImage_PositionBottomRightRepeatX unalignedImage_PositionBottomRightRepeatY unalignedImage_PositionCenterBottomRepeatX unalignedImage_PositionCenterBottomRepeatY unalignedImage_PositionCenterFiftyPercentRepeatY unalignedImage_PositionCenterLeftRepeatY unalignedImage_PositionCenterRepeatY unalignedImage_PositionCenterRightRepeatY unalignedImage_PositionCenterTopRepeatY unalignedImage_RepeatY unalignedImage_Round **5** tests fail with **150%** scale, alignedImage_PositionCenterTopRepeatX alignedImage_RepeatX unalignedImage_Cover unalignedImage_PositionCenterTopRepeatX unalignedImage_RepeatX These tests are marked to skip execution when screen scale is non-integer. - PR: https://git.openjdk.java.net/jfx/pull/344
Re: RFR: [WIP] 8088739: [TestBug] RegionBackgroundImageUITest fail with timeout error [v3]
> Each test in RegionBackgroundImageUITest makes several calls to > `robot.getPixelColor()` on App thread. Due to this each test requires more > than **60** seconds for execution. > > Fix is to save a screen capture of Scene (on App thread) and then read pixel > color from the screen capture(not on app thread). This reduces execution time > of each test to less than **3** seconds. > Ideally with this fix(commit#1) all the tests should pass. All tests do pass > on Windows and Linux but three tests fail on Mac, which used to pass without > this change. > - RegionBackgroundImageUITest.unalignedImage > - RegionBackgroundFillUITest.testScenario2 > - RegionBackgroundFillUITest.testScenario3 > > commit#2 solves the above problem. Solution is to fallback to test color > again by reading it using `robot.getPixelColor()` on App thread when a test > fails. > > One test RegionBackgroundImageUITest.unalignedImage_Cover, fails only on Mac > platform, before and after this fix. > It is reported as a new issue > [JDK-8255679](https://bugs.openjdk.java.net/browse/JDK-8255679) > > This is a test fix and affects only the tests that extend from > `RegionUITestBase` test class and does not affect other tests. > Verified that `RegionBackgroundImageUITest` and `RegionBackgroundFillUITest` > tests pass on all three platforms(except > RegionBackgroundImageUITest.unalignedImage_Cover which fails on Mac). Ambarish Rapte has updated the pull request incrementally with one additional commit since the last revision: skip tests that fail on windows with non-integer screen scale - Changes: - all: https://git.openjdk.java.net/jfx/pull/344/files - new: https://git.openjdk.java.net/jfx/pull/344/files/f270a0bb..91b8d543 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx&pr=344&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jfx&pr=344&range=01-02 Stats: 38 lines in 1 file changed: 37 ins; 1 del; 0 mod Patch: https://git.openjdk.java.net/jfx/pull/344.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/344/head:pull/344 PR: https://git.openjdk.java.net/jfx/pull/344
Re: RFR: 8089589: [ListView] ScrollBar content moves toward-backward during scrolling. [v2]
On Mon, 12 Apr 2021 09:41:03 GMT, Johan Vos wrote:
>> This PR introduces a refactory for VirtualFlow, fixing a number of issues
>> reported about inconsistent scrolling speed (see
>> https://bugs.openjdk.java.net/browse/JDK-8089589)
>> The problem mentioned in the JBS issue (and in related issues) is that the
>> VirtualFlow implementation depends on cell index and cell count, instead of
>> on pixel count. The latter is unknown when the VirtualFlow is created, and
>> pre-calculating the size of a large set of items would be very expensive.
>> Therefore, this PR uses a combination of a gradual calculation of the total
>> size in pixels (estimatedSize) and a smoothing part that prevents the
>> scrollback to scroll in the reverse direction as the requested change.
>> This PR currently breaks a number of tests that hard-coded depend on a
>> number of evaluations. This is inherit to the approach of this PR: if we
>> want to estimate the total size, we need to do some additional calculations.
>> In this PR, I try to balance between consistent behavior and performance.
>
> Johan Vos has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Process reviewer comments
Code changes are OK. I have noted a few minor comments.
modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java
line 1305:
> 1303: // Otherwise, our goal is to leave the index of the cell at
> the
> 1304: // top consistent, with the same translation etc.
> 1305: if (position != 0 && position != 1 && (currentIndex >=
> cellCount)) {
Comparing a double for equality or inequality is not the best coding practice.
Anyway, I see this pattern throughout this file. We can live with it for now.
modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/VirtualFlowTest.java
line 658:
> 656: * laid out properly.
> 657: */
> 658: @Test
Only new line is added after @Test tag. There are a lot of changes of this type
which I presume were done by the IDE. I think, we can revert these safely as
not to cause unnecessary diffs.
modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/VirtualFlowTest.java
line 1233:
> 1231: private VirtualFlowShim createCircleFlow() {
> 1232: // The second VirtualFlow we are going to test, with 7 cells.
> Each cell
> 1233: // contains a Circle whith a radius that varies between cells.
Typo : "whith" -> "with"
modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/VirtualFlowTest.java
line 1282:
> 1280: flow.scrollPixels(-50);
> 1281: double neg = flow.getPosition();
> 1282: assertFalse("Moving in negative direction should not decrease
> position", neg > pos);
"decrease" in log message should be "increase"
modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/VirtualFlowTest.java
line 1294:
> 1292: vf.scrollPixels(-50);
> 1293: double neg = vf.getPosition();
> 1294: assertFalse("Moving in negative direction should not decrease
> position", neg > pos);
"decrease" in log message should be "increase"
-
PR: https://git.openjdk.java.net/jfx/pull/398
Re: RFR: [WIP] 8088739: [TestBug] RegionBackgroundImageUITest fail with timeout error [v4]
> Each test in RegionBackgroundImageUITest makes several calls to > `robot.getPixelColor()` on App thread. Due to this each test requires more > than **60** seconds for execution. > > Fix is to save a screen capture of Scene (on App thread) and then read pixel > color from the screen capture(not on app thread). This reduces execution time > of each test to less than **3** seconds. > Ideally with this fix(commit#1) all the tests should pass. All tests do pass > on Windows and Linux but three tests fail on Mac, which used to pass without > this change. > - RegionBackgroundImageUITest.unalignedImage > - RegionBackgroundFillUITest.testScenario2 > - RegionBackgroundFillUITest.testScenario3 > > commit#2 solves the above problem. Solution is to fallback to test color > again by reading it using `robot.getPixelColor()` on App thread when a test > fails. > > One test RegionBackgroundImageUITest.unalignedImage_Cover, fails only on Mac > platform, before and after this fix. > It is reported as a new issue > [JDK-8255679](https://bugs.openjdk.java.net/browse/JDK-8255679) > > This is a test fix and affects only the tests that extend from > `RegionUITestBase` test class and does not affect other tests. > Verified that `RegionBackgroundImageUITest` and `RegionBackgroundFillUITest` > tests pass on all three platforms(except > RegionBackgroundImageUITest.unalignedImage_Cover which fails on Mac). Ambarish Rapte has updated the pull request incrementally with one additional commit since the last revision: skip test on mac that fails only on mac - Changes: - all: https://git.openjdk.java.net/jfx/pull/344/files - new: https://git.openjdk.java.net/jfx/pull/344/files/91b8d543..1a30b63c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx&pr=344&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jfx&pr=344&range=02-03 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jfx/pull/344.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/344/head:pull/344 PR: https://git.openjdk.java.net/jfx/pull/344
Re: RFR: 8208088: Memory Leak in ControlAcceleratorSupport [v6]
On Mon, 12 Apr 2021 08:40:56 GMT, Ambarish Rapte wrote: >> The method `ControlAcceleratorSupport.doAcceleratorInstall(final List> extends MenuItem> items, final Scene scene)` adds a `ChangeListener` on >> `MenuItem.acceleratorProperty()`. This listener is not removed when the >> MenuItem is removed from scenegraph. >> Adding and removing a MenuItem results in multiple copies of the listener >> added to MenuItem.acceleratorProperty(). >> >> Fix is to remove the listener when MenuItem is removed. >> Fix can be verified by checking the number of instances of >> `ControlAcceleratorSupport.lambda` using _jvisualvm_. >> Without this fix, the number of `ControlAcceleratorSupport.lambda` increase >> in multiple of number of MenuItems being removed and added back. >> With fix, the count is always same as number of MenuItems in scenegraph. >> >> Also there is another ListChangeListener added to a >> `ObservableList items` in the method >> `ControlAcceleratorSupport.doAcceleratorInstall(final >> ObservableList items, final Scene scene)`. There was a TODO note >> to remove this listener. >> This listener is added on `MenuBarButton.getItems()` and not on >> `Menu.getItems()`. This `MenuBarButton` is created by `MenuBarSkin` to show >> a `Menu`. This `MenuBarButton` gets disposed when the related `Menu` is >> removed from scenegraph, and so the added `ListChangeListener` gets GCed. >> Hence it is not required to explicitly remove the listener. >> Added a comment explaining this behavior in place of the TODO. > > Ambarish Rapte has updated the pull request incrementally with one additional > commit since the last revision: > > remove indirect test Marked as reviewed by aghaisas (Reviewer). - PR: https://git.openjdk.java.net/jfx/pull/429
Re: RFR: 8089589: [ListView] ScrollBar content moves toward-backward during scrolling. [v2]
On Mon, 12 Apr 2021 09:41:03 GMT, Johan Vos wrote:
>> This PR introduces a refactory for VirtualFlow, fixing a number of issues
>> reported about inconsistent scrolling speed (see
>> https://bugs.openjdk.java.net/browse/JDK-8089589)
>> The problem mentioned in the JBS issue (and in related issues) is that the
>> VirtualFlow implementation depends on cell index and cell count, instead of
>> on pixel count. The latter is unknown when the VirtualFlow is created, and
>> pre-calculating the size of a large set of items would be very expensive.
>> Therefore, this PR uses a combination of a gradual calculation of the total
>> size in pixels (estimatedSize) and a smoothing part that prevents the
>> scrollback to scroll in the reverse direction as the requested change.
>> This PR currently breaks a number of tests that hard-coded depend on a
>> number of evaluations. This is inherit to the approach of this PR: if we
>> want to estimate the total size, we need to do some additional calculations.
>> In this PR, I try to balance between consistent behavior and performance.
>
> Johan Vos has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Process reviewer comments
modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/VirtualFlowTest.java
line 1315:
> 1313: s1 = sb.getLayoutY();
> 1314: newDelta = newPosition - position;
> 1315: System.err.println("s0 = "+s0+", s1 = "+s1);
I suggest to comment out all System.err.println() calls from this loop.
It might be useful while debugging individual failing test, but keeping them on
for every test run will simply flood the log.
Similar logs were fixed for stdout earlier. Please refer -
https://bugs.openjdk.java.net/browse/JDK-8255497
-
PR: https://git.openjdk.java.net/jfx/pull/398
Re: RFR: 8264127: ListCell editing status is true, when index changes while editing [v4]
On Wed, 14 Apr 2021 11:58:14 GMT, Florian Kirmaier wrote: >> Fixing ListCell editing status is true, when index changes while editing. > > Florian Kirmaier has updated the pull request incrementally with one > additional commit since the last revision: > > 8264127: > Added checks, whether the correction ammount of editStart/cancel events are > triggered - there's still a failing transition (and not covered by the latest test ;) `cellIndex == editingIndex` to `cellIndex == -1` - as to your fix: don't see a difference between calling `updateEditing `before/after `updateFocus` - simply moving it after the if block seems to solve all toEditing issues That said: I'm aware that getting this right is tedious and a bit hard, not because it's rocket science but because we need to cover the corner cases as completely as possible (and often fail in doing so, me included :) Chances are that if we detect a problem with a transition "-1 -> 1", there's a similar problem in the invers "1 -> -1". Even if there isn't, we should make sure there isn't by adding a test. That's why I personally prefer writing dedicated parameterized tests (vs. adding to the existing general purpose test), here in cell/editingIndex. Don't know if you noticed, yesterday I attached such a test to the issue (had to write it for [JDK-8265206](https://bugs.openjdk.java.net/browse/JDK-8265206) anyway, so no big deal to replace TableCell with ListCell) - whether you take it or adjust yours accordingly doesn't matter, as long as the coverage is complete. BTW, in TableCell the remaining issue (editingIndex -> -1) seems to be related to updateEditing backing out if the current index is -1. It probably should cleanup if in editing state (didn't try yet). - PR: https://git.openjdk.java.net/jfx/pull/441
Re: OpenJFX custom build - Java application crash (semi-related to 8262276)
I’m marking the PR (https://github.com/openjdk/jfx/pull/461) as ready for review. You can add your evaluation in the PR. — Arun Joseph
RFR: 8263788: JavaFX application freezes completely after some time when using the WebView
Issue: Java application (with WebView) will completely freeze after using it for a while. Fix: Use native isMainThread functions instead of JNI call. - Commit messages: - Remove platform check in ThreadingWin - Remove fwkIsMainThread - 8263788: JavaFX application freezes completely after some time when using the WebView Changes: https://git.openjdk.java.net/jfx/pull/461/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx&pr=461&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8263788 Stats: 38 lines in 3 files changed: 15 ins; 18 del; 5 mod Patch: https://git.openjdk.java.net/jfx/pull/461.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/461/head:pull/461 PR: https://git.openjdk.java.net/jfx/pull/461
Re: RFR: 8258777: SkinBase: add api to un-/register invalidation-/listChange listeners [v9]
On Mon, 12 Apr 2021 14:45:58 GMT, Jeanette Winzenburg
wrote:
>> Changes in Lambda..Handler:
>> - added api and implemenation to support invalidation and listChange
>> listeners in the same way as changeListeners
>> - added java doc
>> - added tests
>>
>> Changes in SkinBase
>> - added api (and implementation delegating to the handler)
>> - copied java doc from the change listener un/register methods
>> - added tests to verify that the new (and old) api is indeed delegating to
>> the handler
>>
>> Note that the null handling is slightly extended: all methods now can handle
>> both null consumers (as before) and null observables (new) - this allows
>> simplified code on rewiring "path" properties (see reference example in the
>> issue)
>
> Jeanette Winzenburg has updated the pull request incrementally with one
> additional commit since the last revision:
>
> changed description as suggested in review
I finished reviewing the API changes, and it all looks good. I noted a
follow-up bug that I need to file.
I'll review the CSR (to make sure it matches exactly the latest API docs in the
code) in parallel with reviewing the implementation.
modules/javafx.controls/src/main/java/javafx/scene/control/SkinBase.java line
219:
> 217: * @param observable the observable to observe for change events,
> may be {@code null}
> 218: * @param operation the operation to perform when the observable
> sends a change event,
> 219: * may be {@code null}
This method is missing the `@since 9` javadoc tag. This is a preexisting bug;
this method was missed when
[JDK-8135312](https://bugs.openjdk.java.net/browse/JDK-8135312) was fixed. It's
tempting to fold it into this RFE, since you are rewriting the API docs for
this method, but I'll file a separate bug instead.
-
PR: https://git.openjdk.java.net/jfx/pull/409
Re: RFR: 8258777: SkinBase: add api to un-/register invalidation-/listChange listeners [v2]
On Wed, 24 Mar 2021 23:30:03 GMT, Nir Lisker wrote:
>> Jeanette Winzenburg has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> fixed missing/incorrect @since tags in new api doc
>
> modules/javafx.controls/src/main/java/javafx/scene/control/SkinBase.java line
> 273:
>
>> 271: * @since 17
>> 272: */
>> 273: protected final Consumer
>> unregisterInvalidationListeners(Observable observable) {
>
> Is `null` check on `observable` needed?
`unregisterInvalidationListeners` does the `null` check
-
PR: https://git.openjdk.java.net/jfx/pull/409
Re: RFR: 8258777: SkinBase: add api to un-/register invalidation-/listChange listeners [v9]
On Mon, 12 Apr 2021 14:45:58 GMT, Jeanette Winzenburg wrote: >> Changes in Lambda..Handler: >> - added api and implemenation to support invalidation and listChange >> listeners in the same way as changeListeners >> - added java doc >> - added tests >> >> Changes in SkinBase >> - added api (and implementation delegating to the handler) >> - copied java doc from the change listener un/register methods >> - added tests to verify that the new (and old) api is indeed delegating to >> the handler >> >> Note that the null handling is slightly extended: all methods now can handle >> both null consumers (as before) and null observables (new) - this allows >> simplified code on rewiring "path" properties (see reference example in the >> issue) > > Jeanette Winzenburg has updated the pull request incrementally with one > additional commit since the last revision: > > changed description as suggested in review I filed [JDK-8265277](https://bugs.openjdk.java.net/browse/JDK-8265277) to add the missing `@since 9` javadoc tag. - PR: https://git.openjdk.java.net/jfx/pull/409
Re: RFR: 8258777: SkinBase: add api to un-/register invalidation-/listChange listeners [v2]
On Wed, 24 Mar 2021 23:51:37 GMT, Nir Lisker wrote:
>> Jeanette Winzenburg has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> fixed missing/incorrect @since tags in new api doc
>
> modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/LambdaMultiplePropertyChangeListenerHandler.java
> line 63:
>
>> 61: */
>> 62: public final class LambdaMultiplePropertyChangeListenerHandler {
>> 63: // FIXME: name doesn't fit after widening to support more notification
>> event types
>
> Maybe `MultipleObservableListenersHandler`? I didn't look at the class too
> much.
I like `MultipleObservableListenersHandler` or
`LambdaMultipleObservableListenersHandler` if you want to keep `Lambda` in the
name (not sure it matters much).
-
PR: https://git.openjdk.java.net/jfx/pull/409
Re: RFR: 8264064: Cross compile JavaFX graphics and controls modules for Windows AArch64 (ARM64) [v3]
> This is a proposal for cross compiling JavaFX base modules (excluding media
> and webkit) for Windows AArch64 (ARM64).
>
> Main changes:
> - prismES2 native compilation is moved under IS_INCLUDE_ES2 condition
> - HOST_ARCH and TARGET_ARCH are retrieved from ext.OS_ARCH and
> ext.TARGET_ARCH using substitution aarch64 to arm64 and amd64 to x64
> - VCARCH is set to "${HOST_ARCH}_${TARGET_ARCH}" architecture for cross
> compilation. VCARCH is set to x64 for amd64 target architecture (according to
> the [vcvarsall.bat
> doc](https://docs.microsoft.com/en-us/cpp/build/building-on-the-command-line?view=msvc-160#developer_command_file_locations)
> x64 and amd64 are interchangeable)
> - arm64 rc.exe and fxc.exe execution fails on non arm64 host. The fix checks
> that and falls back to host rc.exe and fxc.exe. The right way would be to use
> rc.exe and fxc.exe from arm64 but I did not find a right way to run them on
> host system.
>
> I also looked which changes are required to build media module for Windows
> aarch64.
> The main changes could be using:
> - `ARCH=arm64` for building media libs in build.gradle file
> - `-MACHINE:arm64` LDFLAGS in media make files
> - `msvc_build/aarch64/aarch64_include` header files for include,
> `src/aarch64/win64_armasm.S` for ASM_SOURCES, `armasm64.exe` for ML in
> gstreamer/projects/win/glib-lite/Makefile.glib file and corresponding include
> in gstreamer/projects/win/glib-lite/Makefile.gobject file
> - setting `ENABLE_SIMD_SSE2` to 0 in ColorConverter.c in the similar way how
> it is done for Apple Silicon port
>
> In this way the media is build but it is crashed when I run a JavaFX sample
> with video.
> Is it possible to send the media Windows aarch64 port to review and
> investigate the crash in the separate fix?
Alexander Scherbatiy has updated the pull request incrementally with one
additional commit since the last revision:
Add CONVERTED_OS_ARCH and CONVERTED_TARGET_ARCH variables
-
Changes:
- all: https://git.openjdk.java.net/jfx/pull/439/files
- new: https://git.openjdk.java.net/jfx/pull/439/files/38184783..c266aed4
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jfx&pr=439&range=02
- incr: https://webrevs.openjdk.java.net/?repo=jfx&pr=439&range=01-02
Stats: 41 lines in 2 files changed: 19 ins; 9 del; 13 mod
Patch: https://git.openjdk.java.net/jfx/pull/439.diff
Fetch: git fetch https://git.openjdk.java.net/jfx pull/439/head:pull/439
PR: https://git.openjdk.java.net/jfx/pull/439
Re: RFR: 8264064: Cross compile JavaFX graphics and controls modules for Windows AArch64 (ARM64) [v2]
On Wed, 14 Apr 2021 15:02:06 GMT, Johan Vos wrote: >> Yes, this does seem like a better plan. Should this be done as a follow-on >> or do you want to see it done now? One reason I ask is that my PR #462 >> (which is now approved, but waiting re-review) does something similar to >> `getWinArch()` to determine whether we are running on aarch64 or not and >> should be modified as well. > > In that case, a follow-up seems best (we can tackle mac/win/linux aarch64 > simultaneously then) I added CONVERTED_OS_ARCH, CONVERTED_TARGET_ARCH variables, and getConvertedOsArch(arch) function to build.gradle. I think there should be the better prefix name for updated os arch but what comes in my mind is converted or unified. Building jfx with the fix on windows 32, 64 bits and arm64 cross compilation outputs the following variables in the log: # Win 32 bits OS_ARCH: x86 TARGET_ARCH: x86 CONVERTED_OS_ARCH: x86 CONVERTED_TARGET_ARCH: x86 # Win 64 bits OS_ARCH: amd64 TARGET_ARCH: amd64 CONVERTED_OS_ARCH: x64 CONVERTED_TARGET_ARCH: x64 # Win arm64 cross compilation OS_ARCH: amd64 TARGET_ARCH: aarch64 CONVERTED_OS_ARCH: x64 CONVERTED_TARGET_ARCH: arm64 - PR: https://git.openjdk.java.net/jfx/pull/439
Re: RFR: 8258777: SkinBase: add api to un-/register invalidation-/listChange listeners [v2]
On Thu, 15 Apr 2021 12:55:56 GMT, Kevin Rushforth wrote:
>> modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/LambdaMultiplePropertyChangeListenerHandler.java
>> line 63:
>>
>>> 61: */
>>> 62: public final class LambdaMultiplePropertyChangeListenerHandler {
>>> 63: // FIXME: name doesn't fit after widening to support more notification
>>> event types
>>
>> Maybe `MultipleObservableListenersHandler`? I didn't look at the class too
>> much.
>
> I like `MultipleObservableListenersHandler` or
> `LambdaMultipleObservableListenersHandler` if you want to keep `Lambda` in
> the name (not sure it matters much).
sorry, somehow this comment didn't reach my consciousness ;)
`MultipleObservableListenersHandler` sounds fine. Would like to extract the
rename to a follow-up issue, though, because it's not only used by SkinBase.
-
PR: https://git.openjdk.java.net/jfx/pull/409
Re: RFR: 8264127: ListCell editing status is true, when index changes while editing [v4]
On Wed, 14 Apr 2021 11:58:14 GMT, Florian Kirmaier wrote: >> Fixing ListCell editing status is true, when index changes while editing. > > Florian Kirmaier has updated the pull request incrementally with one > additional commit since the last revision: > > 8264127: > Added checks, whether the correction ammount of editStart/cancel events are > triggered curious: do you understand the dependency of update sequence (updateEditing relative to updateFocus)? What I see is - `focusIndex == cellIndex` and calling updateEditing before updateFocus - all toEditing transitions are failing due to corrupting list editing state (its editingIndex seems to be changed to -1, that is effectively canceling the edit) - `focusIndex == editingIndex` and calling updateEditing after updateFocus - all offEditing transitions are failing due to corrupting list editing state TableCell does the latter and has no dependency on focus state - at least none I can see ;) - PR: https://git.openjdk.java.net/jfx/pull/441
Re: RFR: 8264064: Cross compile JavaFX graphics and controls modules for Windows AArch64 (ARM64) [v3]
On Thu, 15 Apr 2021 13:56:12 GMT, Alexander Scherbatiy
wrote:
>> This is a proposal for cross compiling JavaFX base modules (excluding media
>> and webkit) for Windows AArch64 (ARM64).
>>
>> Main changes:
>> - prismES2 native compilation is moved under IS_INCLUDE_ES2 condition
>> - HOST_ARCH and TARGET_ARCH are retrieved from ext.OS_ARCH and
>> ext.TARGET_ARCH using substitution aarch64 to arm64 and amd64 to x64
>> - VCARCH is set to "${HOST_ARCH}_${TARGET_ARCH}" architecture for cross
>> compilation. VCARCH is set to x64 for amd64 target architecture (according
>> to the [vcvarsall.bat
>> doc](https://docs.microsoft.com/en-us/cpp/build/building-on-the-command-line?view=msvc-160#developer_command_file_locations)
>> x64 and amd64 are interchangeable)
>> - arm64 rc.exe and fxc.exe execution fails on non arm64 host. The fix checks
>> that and falls back to host rc.exe and fxc.exe. The right way would be to
>> use rc.exe and fxc.exe from arm64 but I did not find a right way to run them
>> on host system.
>>
>> I also looked which changes are required to build media module for Windows
>> aarch64.
>> The main changes could be using:
>> - `ARCH=arm64` for building media libs in build.gradle file
>> - `-MACHINE:arm64` LDFLAGS in media make files
>> - `msvc_build/aarch64/aarch64_include` header files for include,
>> `src/aarch64/win64_armasm.S` for ASM_SOURCES, `armasm64.exe` for ML in
>> gstreamer/projects/win/glib-lite/Makefile.glib file and corresponding
>> include in gstreamer/projects/win/glib-lite/Makefile.gobject file
>> - setting `ENABLE_SIMD_SSE2` to 0 in ColorConverter.c in the similar way how
>> it is done for Apple Silicon port
>>
>> In this way the media is build but it is crashed when I run a JavaFX sample
>> with video.
>> Is it possible to send the media Windows aarch64 port to review and
>> investigate the crash in the separate fix?
>
> Alexander Scherbatiy has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Add CONVERTED_OS_ARCH and CONVERTED_TARGET_ARCH variables
build.gradle line 264:
> 262: case "amd64" : return "x64"
> 263: default: return arch
> 264: }
I think a function that gets the "converted" arch is very useful, but I'm not
sure what the resulting value should be. In case of arm64/aarch64, I have s
slight preference for aarch64, but I'm not against arm64.
-
PR: https://git.openjdk.java.net/jfx/pull/439
Re: RFR: 8264064: Cross compile JavaFX graphics and controls modules for Windows AArch64 (ARM64) [v3]
On Thu, 15 Apr 2021 15:34:42 GMT, Johan Vos wrote: >> Alexander Scherbatiy has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Add CONVERTED_OS_ARCH and CONVERTED_TARGET_ARCH variables > > build.gradle line 264: > >> 262: case "amd64" : return "x64" >> 263: default: return arch >> 264: } > > I think a function that gets the "converted" arch is very useful, but I'm not > sure what the resulting value should be. In case of arm64/aarch64, I have s > slight preference for aarch64, but I'm not against arm64. This does seem useful, although in this case, wasn't `arm64` chosen for compatibility with the name in the Microsoft VisualStudio distro? If so, then this part of the most recent change might be Windows-specific. It might be better to revert this (going back to your previous fix in `win.gradle`) until we can take a more holistic look at this. - PR: https://git.openjdk.java.net/jfx/pull/439
Re: RFR: 8264990: WebEngine crashes with segfault when not loaded through system classloader [v2]
On Thu, 15 Apr 2021 05:34:08 GMT, Arun Joseph wrote: >> Matthias Bläsing has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Revert changes to thread attachment introduced in second commit >> >> Thread attachment is handled in >> >> jfx/modules/javafx.web/src/main/native/Source/WebKitLegacy/Storage/StorageThread.cpp >> and thus does not need to be modified. > > tests/system/src/test/java/test/com/sun/webkit/LocalStorageAccessTest.java > line 36: > >> 34: /** >> 35: * @test >> 36: * @bug 9069811 > > The changes looks good. The bug id is not modified at this point. Fixed, sorry I missed it after the issue was made public. - PR: https://git.openjdk.java.net/jfx/pull/458
Re: RFR: 8264990: WebEngine crashes with segfault when not loaded through system classloader [v3]
> The functions from FileSystemJava are called from different threads the > root problem manifests because the JNI FindClass function behaves > differently when called from a context that is the ancestor of a java > frame compared to when called in isolation. > > A segmentation fault is observed when local storage of a webview is > accessed. At that time a new native thread is spun up and that sets up > the local storage, by calling into the JVM via > WTF::FileSystem::makeAllDirectories. At that point GetFileSystemClass is > invoked to get a referenc to the java implementation of the FileSystem. > As this is is called from a new native thread (no java context > available), JNI uses the system classloader to locate the class. This > fails if the JavaFX modules are not on the boot module/class path. > > Instead on relying on fetching the class reference everytime it is > needed, this change fetches it once when the JavaFX library is loaded > and stores it in the WTF namespace. > > In addition to this it was observed, that there is no attachment to the > JVM done when calling into the filesystem. No fault was observed, but > the JNI specs indicate, that the JNIEnv interface is only valid when > attached. Matthias Bläsing has updated the pull request incrementally with one additional commit since the last revision: Review fix: Use correct bug id - Changes: - all: https://git.openjdk.java.net/jfx/pull/458/files - new: https://git.openjdk.java.net/jfx/pull/458/files/c9ec47c9..e1de314d Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx&pr=458&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jfx&pr=458&range=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jfx/pull/458.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/458/head:pull/458 PR: https://git.openjdk.java.net/jfx/pull/458
Re: RFR: 8088739: [TestBug] RegionBackgroundImageUITest fail with timeout error [v4]
On Mon, 2 Nov 2020 14:12:25 GMT, Kevin Rushforth wrote: > this might be a bug in Robot itself It seems like a robot issue. These failures can also be observed without this change and with increased timeout (7 ms). > The fix for [JDK-8170026](https://bugs.openjdk.java.net/browse/JDK-8170026) > addressed this by skipping the tests that were sensitive to screen scale (14 > of them). You either need to do something similar for > `RegionBackgroundImageUITest` Here also I made same change to skip the failing tests(listed in previous comment). Please take a look. - PR: https://git.openjdk.java.net/jfx/pull/344
Re: RFR: 8264064: Cross compile JavaFX graphics and controls modules for Windows AArch64 (ARM64) [v4]
> This is a proposal for cross compiling JavaFX base modules (excluding media
> and webkit) for Windows AArch64 (ARM64).
>
> Main changes:
> - prismES2 native compilation is moved under IS_INCLUDE_ES2 condition
> - HOST_ARCH and TARGET_ARCH are retrieved from ext.OS_ARCH and
> ext.TARGET_ARCH using substitution aarch64 to arm64 and amd64 to x64
> - VCARCH is set to "${HOST_ARCH}_${TARGET_ARCH}" architecture for cross
> compilation. VCARCH is set to x64 for amd64 target architecture (according to
> the [vcvarsall.bat
> doc](https://docs.microsoft.com/en-us/cpp/build/building-on-the-command-line?view=msvc-160#developer_command_file_locations)
> x64 and amd64 are interchangeable)
> - arm64 rc.exe and fxc.exe execution fails on non arm64 host. The fix checks
> that and falls back to host rc.exe and fxc.exe. The right way would be to use
> rc.exe and fxc.exe from arm64 but I did not find a right way to run them on
> host system.
>
> I also looked which changes are required to build media module for Windows
> aarch64.
> The main changes could be using:
> - `ARCH=arm64` for building media libs in build.gradle file
> - `-MACHINE:arm64` LDFLAGS in media make files
> - `msvc_build/aarch64/aarch64_include` header files for include,
> `src/aarch64/win64_armasm.S` for ASM_SOURCES, `armasm64.exe` for ML in
> gstreamer/projects/win/glib-lite/Makefile.glib file and corresponding include
> in gstreamer/projects/win/glib-lite/Makefile.gobject file
> - setting `ENABLE_SIMD_SSE2` to 0 in ColorConverter.c in the similar way how
> it is done for Apple Silicon port
>
> In this way the media is build but it is crashed when I run a JavaFX sample
> with video.
> Is it possible to send the media Windows aarch64 port to review and
> investigate the crash in the separate fix?
Alexander Scherbatiy has updated the pull request incrementally with one
additional commit since the last revision:
Revert back fix with CONVERTED_OS_ARCH and CONVERTED_TARGET_ARCH variables
-
Changes:
- all: https://git.openjdk.java.net/jfx/pull/439/files
- new: https://git.openjdk.java.net/jfx/pull/439/files/c266aed4..b898bb49
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jfx&pr=439&range=03
- incr: https://webrevs.openjdk.java.net/?repo=jfx&pr=439&range=02-03
Stats: 41 lines in 2 files changed: 9 ins; 19 del; 13 mod
Patch: https://git.openjdk.java.net/jfx/pull/439.diff
Fetch: git fetch https://git.openjdk.java.net/jfx pull/439/head:pull/439
PR: https://git.openjdk.java.net/jfx/pull/439
Re: RFR: 8264064: Cross compile JavaFX graphics and controls modules for Windows AArch64 (ARM64) [v3]
On Thu, 15 Apr 2021 15:57:32 GMT, Kevin Rushforth wrote: >> build.gradle line 264: >> >>> 262: case "amd64" : return "x64" >>> 263: default: return arch >>> 264: } >> >> I think a function that gets the "converted" arch is very useful, but I'm >> not sure what the resulting value should be. In case of arm64/aarch64, I >> have s slight preference for aarch64, but I'm not against arm64. > > This does seem useful, although in this case, wasn't `arm64` chosen for > compatibility with the name in the Microsoft VisualStudio distro? If so, then > this part of the most recent change might be Windows-specific. It might be > better to revert this (going back to your previous fix in `win.gradle`) until > we can take a more holistic look at this. Yes, the arm64 is used as an OS arch for Visual Studio vcvarsall.bat and x64 as a part of a path to arch specific Visual Studio tools on Windows. I reverted the fix with converted os arch variables back. - PR: https://git.openjdk.java.net/jfx/pull/439
Re: RFR: 8264990: WebEngine crashes with segfault when not loaded through system classloader [v2]
On Mon, 12 Apr 2021 16:17:37 GMT, Kevin Rushforth wrote: > @johanvos Do you have a case where it is actually failing as a result of a > thread not being attached? No, I don't have a usecase for that. Hence, as long as there is no trace that can lead to this, I agree this (calling the Attach/Detach) doesn't need to be added. - PR: https://git.openjdk.java.net/jfx/pull/458
Withdrawn: 8264886: WebKit native to Java invocations on wrong thread
On Thu, 8 Apr 2021 12:18:59 GMT, Johan Vos wrote: > Fix for JDK-8264886 This pull request has been closed without being integrated. - PR: https://git.openjdk.java.net/jfx/pull/459
Re: RFR: 8263788: JavaFX application freezes completely after some time when using the WebView
On Mon, 12 Apr 2021 13:10:53 GMT, Arun Joseph wrote:
> Issue: Java application (with WebView) will completely freeze after using it
> for a while.
>
> Fix: Use native isMainThread functions instead of JNI call.
The code changes look good. I still need to test it. Speaking of which, I
presume you've tested this on all three platforms?
I left one question inline.
@johanvos, @arapte, or @guruhb can one of you be the second reviewer?
modules/javafx.web/src/main/native/Source/WTF/wtf/java/MainThreadJava.cpp line
45:
> 43: #elif OS(WINDOWS)
> 44: static ThreadIdentifier mainThread { 0 };
> 45: #endif
Both here and below, where the `isMainThread` is defined, assume that `UNIX`
and `WINDOWS` are the only two options. I presume this is a valid assumption?
-
PR: https://git.openjdk.java.net/jfx/pull/461
Re: [External] : Re: Consistent baseline layout algorithm
I've learned a few new things while working on the proposed new layout
algorithm, and added a few new APIs:
1. A central concept of the new algorithm was the notion of a
text-baseline node, indicated by Node::isTextBaseline(). I've come to
realize that this property should percolate upwards in the scene
graph: if a node has a text-baseline, the node's parent should also be
considered to have a text-baseline. Adding this new behavior works
surprisingly well and produces very intuitive layout results.
2. The default behavior of all layout containers is to pick the first
text-baseline child to derive their own baseline offset. I've added
Node::prefBaselineProperty(), which makes it possible to override this
default selection: layout containers now pick the first child that
reports Node::isPrefBaseline(), and only if there is no such child,
they fall back to Node::isTextBaseline(). Developers can use this
property to fine-tune their baseline layouts.
3. Optimization: Controls that contain text will often consist of a
container of some sort and a javafx.scene.text.Text node within it.
Computing the baseline offset of such a control is very easy with the
new layout algorithm:
public double getBaselineOffset() {
return text.getLayoutBounds().getMinY() + text.getLayoutY() +
text.getBaselineOffset();
}
This works because changing text.layoutY will automatically schedule
another layout pass for all of its parents. Re-layouting all parents
is necessary because changing layoutY can change the effective
baseline offset, and changing the baseline offset of any node can have
layout implications on any of its parents.
However, when we consider the Label control (which is probably among
the most commonly used controls), this can be a bit excessive. Label
controls are often used to display pure text, and as such, we can
often "know" the baseline offset without actually needing to schedule
a second layout pass. This assumption is only correct if the text
within the Label is top-aligned (because if it isn't, the Label
baseline offset can not be known in advance of an actual layout pass).
To leverage this assumption, I've changed the default alignment for
Label to TOP_LEFT (the default alignment of the base class Labeled is
CENTER_LEFT). In most cases, there will be no visual difference
anyway, because I imagine Label controls will seldomly be set to a
minHeight or prefHeight.
This specific scenario will enable an optimization where the first
layout pass of Label will not schedule a second layout pass. It might
be possible to find more such scenarios that can benefit from
fast-path optimizations.
4. In order to get a better understanding of the layout process, I
added additional logging to track layout passes. Then I compared the
current algorithm with the new algorithm by tracking the initial
layout after starting a sample program (i.e. all layout activity until
the first frame is rendered). In the following log, "cumulative layout
passes" means how often layoutChildren() has been invoked on any of
the scene graph nodes. The actual log output includes a tree
visualization of the entire scene graph that is being layouted, which
I've removed for the sake or brevity.
Current algorithm log output:
INFO: Layouting VBox (triggered by scene out-of-pulse), cumulative
layout passes: 49
INFO: Layouting VBox (triggered by scene pulse), cumulative layout passes: 43
INFO: Layouting VBox (triggered by scene pulse), cumulative layout passes: 0
New algorithm log output:
INFO: Layouting VBox (triggered by scene out-of-pulse), cumulative
layout passes: 86
INFO: Layouting VBox (triggered by scene pulse), cumulative layout passes: 19
A major difference is that the current algorithm will often leave the
scene graph in a 'dirty' layout state after running a complete layout
cycle, which necessitates another layout cycle as part of the next
pulse. The new algorithm, however, leaves the scene graph in a clean
layout state after a complete cycle, which takes more work at first,
but saves work that would otherwise be done in the next pulse.
5. Since the new layout algorithm will leave the scene graph in a
clean state, it is not necessary to repeatedly layout the same thing
(like in Node::doLayoutPass()). Cases like these should be identified
and may be changed to single layout invocations.
Overall, I think there's good reason to assume that the proposed
algorithm works and that it produces consistent results for
application developers. At this point it would be useful to know
whether or not to continue with the effort.
Integrated: 8208088: Memory Leak in ControlAcceleratorSupport
On Wed, 17 Mar 2021 17:35:34 GMT, Ambarish Rapte wrote: > The method `ControlAcceleratorSupport.doAcceleratorInstall(final List extends MenuItem> items, final Scene scene)` adds a `ChangeListener` on > `MenuItem.acceleratorProperty()`. This listener is not removed when the > MenuItem is removed from scenegraph. > Adding and removing a MenuItem results in multiple copies of the listener > added to MenuItem.acceleratorProperty(). > > Fix is to remove the listener when MenuItem is removed. > Fix can be verified by checking the number of instances of > `ControlAcceleratorSupport.lambda` using _jvisualvm_. > Without this fix, the number of `ControlAcceleratorSupport.lambda` increase > in multiple of number of MenuItems being removed and added back. > With fix, the count is always same as number of MenuItems in scenegraph. > > Also there is another ListChangeListener added to a `ObservableList > items` in the method `ControlAcceleratorSupport.doAcceleratorInstall(final > ObservableList items, final Scene scene)`. There was a TODO note to > remove this listener. > This listener is added on `MenuBarButton.getItems()` and not on > `Menu.getItems()`. This `MenuBarButton` is created by `MenuBarSkin` to show > a `Menu`. This `MenuBarButton` gets disposed when the related `Menu` is > removed from scenegraph, and so the added `ListChangeListener` gets GCed. > Hence it is not required to explicitly remove the listener. > Added a comment explaining this behavior in place of the TODO. This pull request has now been integrated. Changeset: 05ab7992 Author:Ambarish Rapte URL: https://git.openjdk.java.net/jfx/commit/05ab7992 Stats: 147 lines in 2 files changed: 135 ins; 10 del; 2 mod 8208088: Memory Leak in ControlAcceleratorSupport Reviewed-by: kcr, aghaisas - PR: https://git.openjdk.java.net/jfx/pull/429
