Re: [Approved] RFR: 8200224: Multiple press event when JFXPanel gains focus
On Tue, 26 Nov 2019 13:06:36 GMT, Florian Kirmaier wrote: > The pull request has been updated with additional changes. > > > > Added commits: > - 24385eb8: JDK-8200224 > - e0829ad3: JDK-8200224 > - c190384f: JDK-8200224 > - 17b458b1: JDK-8200224 > > Changes: > - all: https://git.openjdk.java.net/jfx/pull/25/files > - new: https://git.openjdk.java.net/jfx/pull/25/files/44774dfb..24385eb8 > > Webrevs: > - full: https://webrevs.openjdk.java.net/jfx/25/webrev.04 > - incr: https://webrevs.openjdk.java.net/jfx/25/webrev.03-04 > > Issue: https://bugs.openjdk.java.net/browse/JDK-8200224 > Stats: 141 lines in 2 files changed: 123 ins; 11 del; 7 mod > Patch: https://git.openjdk.java.net/jfx/pull/25.diff > Fetch: git fetch https://git.openjdk.java.net/jfx pull/25/head:pull/25 Looks good albeit with minor suggestion regarding presence of bugid in comment Approved by psadhukhan (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/25
Re: [Integrated] RFR: 8193445: JavaFX CSS is applied redundantly leading to significant performance degradation
Changeset: 83eb0a7c Author:Ajit Ghaisas Date: 2019-11-27 07:05:15 + URL: https://git.openjdk.java.net/jfx/commit/83eb0a7c 8193445: JavaFX CSS is applied redundantly leading to significant performance degradation Reviewed-by: kcr, dgrieve ! modules/javafx.graphics/src/main/java/javafx/scene/Node.java + tests/system/src/test/java/test/javafx/scene/QuadraticCssTimeTest.java
Re: [Rev 04] RFR: 8200224: Multiple press event when JFXPanel gains focus
On Tue, 26 Nov 2019 13:06:36 GMT, Florian Kirmaier wrote: > The pull request has been updated with additional changes. > > > > Added commits: > - 24385eb8: JDK-8200224 > - e0829ad3: JDK-8200224 > - c190384f: JDK-8200224 > - 17b458b1: JDK-8200224 > > Changes: > - all: https://git.openjdk.java.net/jfx/pull/25/files > - new: https://git.openjdk.java.net/jfx/pull/25/files/44774dfb..24385eb8 > > Webrevs: > - full: https://webrevs.openjdk.java.net/jfx/25/webrev.04 > - incr: https://webrevs.openjdk.java.net/jfx/25/webrev.03-04 > > Issue: https://bugs.openjdk.java.net/browse/JDK-8200224 > Stats: 141 lines in 2 files changed: 123 ins; 11 del; 7 mod > Patch: https://git.openjdk.java.net/jfx/pull/25.diff > Fetch: git fetch https://git.openjdk.java.net/jfx pull/25/head:pull/25 modules/javafx.swing/src/main/java/javafx/embed/swing/JFXPanel.java line 449: > 448: requestFocus(); > 449: // This fixes JDK-8087914 without causing JDK-8200224 > 450: // It is safe, because in JavaFX only the method > "setFocused(true)" is called, We actually do not clutter source code with bugids, it will be good if it can be removed with proper comments maybe something like "extra simulated mouse pressed event is removed by making the JavaFX scene focused" PR: https://git.openjdk.java.net/jfx/pull/25
Re: [Approved] RFR: 8200224: Multiple press event when JFXPanel gains focus
On Tue, 26 Nov 2019 13:06:36 GMT, Florian Kirmaier wrote: > The pull request has been updated with additional changes. > > > > Added commits: > - 24385eb8: JDK-8200224 > - e0829ad3: JDK-8200224 > - c190384f: JDK-8200224 > - 17b458b1: JDK-8200224 > > Changes: > - all: https://git.openjdk.java.net/jfx/pull/25/files > - new: https://git.openjdk.java.net/jfx/pull/25/files/44774dfb..24385eb8 > > Webrevs: > - full: https://webrevs.openjdk.java.net/jfx/25/webrev.04 > - incr: https://webrevs.openjdk.java.net/jfx/25/webrev.03-04 > > Issue: https://bugs.openjdk.java.net/browse/JDK-8200224 > Stats: 141 lines in 2 files changed: 123 ins; 11 del; 7 mod > Patch: https://git.openjdk.java.net/jfx/pull/25.diff > Fetch: git fetch https://git.openjdk.java.net/jfx pull/25/head:pull/25 All looks good now. As a reminder, this will need a second reviewer. Approved by kcr (Lead). PR: https://git.openjdk.java.net/jfx/pull/25
Re: RFR: 8130738: Add tabSize property to Text and TextFlow
On Wed, 27 Nov 2019 01:17:42 GMT, Scott Palmer wrote: > On Tue, 26 Nov 2019 18:48:38 GMT, Kevin Rushforth wrote: > >> On Tue, 26 Nov 2019 18:40:10 GMT, Scott Palmer wrote: >> >>> On Thu, 7 Nov 2019 14:56:58 GMT, Kevin Rushforth wrote: >>> On Wed, 6 Nov 2019 19:11:48 GMT, Scott Palmer wrote: > Added tabSize property to Text and TextFlow and -fx-tab-size CSS > attribute to both. TextFlow's tab size overrides that of contained Text > nodes. > > > > Commits: > - 68d221c7: 8130738: TextFlow's tab width is static > > Changes: https://git.openjdk.java.net/jfx/pull/32/files > Webrev: https://webrevs.openjdk.java.net/jfx/32/webrev.00 > Issue: https://bugs.openjdk.java.net/browse/JDK-8130738 > Stats: 324 lines in 8 files changed: 260 ins; 0 del; 64 mod > Patch: https://git.openjdk.java.net/jfx/pull/32.diff > Fetch: git fetch https://git.openjdk.java.net/jfx pull/32/head:pull/32 > I should clamp so it reads back as 1 - Fixed. This has implications for binding, etc., so I suspect this is not what we want. I'll look into this more when I review it. NOTE: This will need a CSR in addition to two reviewers. You can file the CSR any time, but I suggest leaving it in Draft state until Phil and I have had a first pass look at the API. >>> >>> The indenting was wrong in that section and I had to make some minor >>> changes, so I corrected the bad indenting. Kevin indicated, "In your >>> specific case, reformatting the methods in the StyleableProperties nested >>> class that are adjacent to code you add or modify seems fine, as long as >>> you only change the indentation and not the line wrapping." >>> >>> I didn't intend to trigger an update to this pull request...I find Git >>> awkward to work with (Mercurial makes more sense to me) so I'm making >>> mistakes as I bumble around. >>> >>> I do expect an update to address the way I've clamped to 1. I agree with >>> Kevin that it is probable wrong. Just waiting for feedback. >> >> I'll take a preliminary look today. I also plan to change the bug title to >> make it more clear that it is an enhancement request, since it currently >> reads like a bug report (I'll also change the CSR and PR title to match). > > I just noticed that Text.toString is missing any indication of the tabSize. > (Found by accident when I search for wrappingWidth to see how the javadocs > were done.) > Is there any rule as to where in the order of properties ", tabSize = X" > should be in the string? I assume it should only be included when not set to > the default value, much like lineSpacing? Yes, I think your idea of including `tabSize` in `Text.toString` when != default is a good one. PR: https://git.openjdk.java.net/jfx/pull/32
Re: RFR: 8130738: Add tabSize property to Text and TextFlow
On Tue, 26 Nov 2019 18:48:38 GMT, Kevin Rushforth wrote: > On Tue, 26 Nov 2019 18:40:10 GMT, Scott Palmer wrote: > >> On Thu, 7 Nov 2019 14:56:58 GMT, Kevin Rushforth wrote: >> >>> On Wed, 6 Nov 2019 19:11:48 GMT, Scott Palmer wrote: >>> Added tabSize property to Text and TextFlow and -fx-tab-size CSS attribute to both. TextFlow's tab size overrides that of contained Text nodes. Commits: - 68d221c7: 8130738: TextFlow's tab width is static Changes: https://git.openjdk.java.net/jfx/pull/32/files Webrev: https://webrevs.openjdk.java.net/jfx/32/webrev.00 Issue: https://bugs.openjdk.java.net/browse/JDK-8130738 Stats: 324 lines in 8 files changed: 260 ins; 0 del; 64 mod Patch: https://git.openjdk.java.net/jfx/pull/32.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/32/head:pull/32 >>> I should clamp so it reads back as 1 - Fixed. >>> >>> This has implications for binding, etc., so I suspect this is not what we >>> want. I'll look into this more when I review it. >>> >>> NOTE: This will need a CSR in addition to two reviewers. You can file the >>> CSR any time, but I suggest leaving it in Draft state until Phil and I have >>> had a first pass look at the API. >> >> The indenting was wrong in that section and I had to make some minor >> changes, so I corrected the bad indenting. Kevin indicated, "In your >> specific case, reformatting the methods in the StyleableProperties nested >> class that are adjacent to code you add or modify seems fine, as long as you >> only change the indentation and not the line wrapping." >> >> I didn't intend to trigger an update to this pull request...I find Git >> awkward to work with (Mercurial makes more sense to me) so I'm making >> mistakes as I bumble around. >> >> I do expect an update to address the way I've clamped to 1. I agree with >> Kevin that it is probable wrong. Just waiting for feedback. > > I'll take a preliminary look today. I also plan to change the bug title to > make it more clear that it is an enhancement request, since it currently > reads like a bug report (I'll also change the CSR and PR title to match). I just noticed that Text.toString is missing any indication of the tabSize. (Found by accident when I search for wrappingWidth to see how the javadocs were done.) Is there any rule as to where in the order of properties ", tabSize = X" should be in the string? I assume it should only be included when not set to the default value, much like lineSpacing? PR: https://git.openjdk.java.net/jfx/pull/32
Re: [Rev 02] RFR: 8130738: TextFlow's tab width is static
On Tue, 26 Nov 2019 17:26:33 GMT, Scott Palmer wrote:
> The pull request has been updated with a complete new set of changes
> (possibly due to a rebase).
>
>
>
> Commits:
> - 254c40de: Merge remote-tracking branch 'upstream/master'
> - a670c4f8: 8130738: TextFlow's tab width is static
> - 68d221c7: 8130738: TextFlow's tab width is static
>
> Changes: https://git.openjdk.java.net/jfx/pull/32/files
> Webrev: https://webrevs.openjdk.java.net/jfx/32/webrev.02
> Issue: https://bugs.openjdk.java.net/browse/JDK-8130738
> Stats: 325 lines in 8 files changed: 261 ins; 0 del; 64 mod
> Patch: https://git.openjdk.java.net/jfx/pull/32.diff
> Fetch: git fetch https://git.openjdk.java.net/jfx pull/32/head:pull/32
I did a first pass review focusing mostly on the public API. Once we get that
solidified I'll look at the implementation and tests more closely. Also, once
the public API is done, you can update the CSR with the API.
modules/javafx.graphics/src/main/docs/javafx/scene/doc-files/cssref.html line
3035:
> 3034:
> 3035:
> 3036: -fx-text-alignment
the `` should be on the next line (once you do, you can remove the trailing
white space that will then be present).
modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayout.java
line 655:
> 654: if (spaces < 1)
> 655: spaces = 1;
> 656: if (tabSize != spaces) {
Please surround the statement with curly braces (our coding style is to always
surround the target of a conditional in curly braces unless it is on the same
line).
modules/javafx.graphics/src/main/java/javafx/scene/text/Text.java line 1289:
> 1288: * @since 14
> 1289: */
> 1290: public final int getTabSize() {
The javadoc property support only requires one of the
setXxxx/getXxxx/Property methods to have a javadoc comment block. The
javadoc tool takes care of documenting the property itself and all three
methods. See `wrappingWidth` for a good example.
modules/javafx.graphics/src/main/java/javafx/scene/text/Text.java line 1302:
> 1301: * @since 14
> 1302: */
> 1303: public final void setTabSize(int spaces) {
Same comment as on the set method. You can move the comment about values being
clamped to 1 to the property method (which will then be the only method with
javadoc comments). If we use my recommendation of clamping on usage, I
recommend the following language, borrowed from `Node::opacity`:
Values less than 1 are treated as 1.
modules/javafx.graphics/src/main/java/javafx/scene/text/Text.java line 1895:
> 1894: }
> 1895: @Override public void set(int v) { super.set((v <
> 1) ? 1 : v); }
> 1896: @Override protected void invalidated() {
For mutable properties, we usually clamp on usage, so that we don't have
problems binding to the value. This preserves the invariant that `set(val);
get() == val` for all values. If that is what we end up doing, then this
overridden method should be removed.
modules/javafx.graphics/src/main/java/javafx/scene/text/TextFlow.java line 515:
> 514: * @since 14
> 515: */
> 516: public final void setTabSize(int spaces) {
Same comments about the javadoc comments as for `Text.java`
modules/javafx.graphics/src/main/java/javafx/scene/text/TextFlow.java line 528:
> 527: }
> 528: @Override public void set(int v) { super.set((v < 1) ? 1
> : v); }
> 529: @Override protected void invalidated() {
Same clamp-on-use comment as in `Text.java`
modules/javafx.graphics/src/test/java/test/com/sun/javafx/pgstub/StubTextLayout.java
line 200:
> 199: if (spaces < 1)
> 200: spaces = 1;
> 201: if (tabSize != spaces) {
Either surround with braces or put onto the same line as the `if` test.
modules/javafx.graphics/src/test/java/test/javafx/scene/text/TextTest.java line
271:
> 270: }
> 271: }
> 272: }
In addition to the above test, I recommend some (very basic) tests of the
setter / getting / property methods.
modules/javafx.graphics/src/test/java/test/javafx/scene/text/TextTest.java line
251:
> 250: tk.firePulse();
> 251: assertEquals(text.getTabSize(),8);
> 252: // initial width with default 8-space tab
This is backwards. The expected value should be the first argument.
PR: https://git.openjdk.java.net/jfx/pull/32
Re: [Integrated] RFR: 8230610: Upgrade GStreamer to the latest 1.16.1
Changeset: 798afbca Author:Alexander Matveev Date: 2019-11-26 22:33:00 + URL: https://git.openjdk.java.net/jfx/commit/798afbca 8230610: Upgrade GStreamer to version 1.16.1 8230609: Upgrade glib to version 2.62.2 Reviewed-by: kcr, jvos ! modules/javafx.media/src/main/legal/glib.md ! modules/javafx.media/src/main/legal/gstreamer.md ! modules/javafx.media/src/main/native/gstreamer/3rd_party/glib/build/osx/config.h ! modules/javafx.media/src/main/native/gstreamer/3rd_party/glib/build/osx/glibconfig.h ! modules/javafx.media/src/main/native/gstreamer/3rd_party/glib/build/win32/dirent/dirent.c ! modules/javafx.media/src/main/native/gstreamer/3rd_party/glib/build/win32/dirent/dirent.h ! modules/javafx.media/src/main/native/gstreamer/3rd_party/glib/build/win32/vs100/config.h ! modules/javafx.media/src/main/native/gstreamer/3rd_party/glib/build/win32/vs100/glib-lite.def - modules/javafx.media/src/main/native/gstreamer/3rd_party/glib/build/win32/vs100/glib-liteD.def ! modules/javafx.media/src/main/native/gstreamer/3rd_party/glib/build/win32/vs100/glibconfig.h ! modules/javafx.media/src/main/native/gstreamer/3rd_party/glib/build/win32/vs100/gmoduleconf.h ! modules/javafx.media/src/main/native/gstreamer/3rd_party/glib/build/win32/vs100/msvc_recommended_pragmas.h ! modules/javafx.media/src/main/native/gstreamer/3rd_party/glib/glib/deprecated/gthread.h ! modules/javafx.media/src/main/native/gstreamer/3rd_party/glib/glib/galloca.h ! modules/javafx.media/src/main/native/gstreamer/3rd_party/glib/glib/garray.c ! modules/javafx.media/src/main/native/gstreamer/3rd_party/glib/glib/garray.h ! modules/javafx.media/src/main/native/gstreamer/3rd_party/glib/glib/gasyncqueue.c ! modules/javafx.media/src/main/native/gstreamer/3rd_party/glib/glib/gasyncqueue.h ! modules/javafx.media/src/main/native/gstreamer/3rd_party/glib/glib/gatomic.c ! modules/javafx.media/src/main/native/gstreamer/3rd_party/glib/glib/gatomic.h ! modules/javafx.media/src/main/native/gstreamer/3rd_party/glib/glib/gbacktrace.c ! modules/javafx.media/src/main/native/gstreamer/3rd_party/glib/glib/gbase64.c ! modules/javafx.media/src/main/native/gstreamer/3rd_party/glib/glib/gbitlock.c ! modules/javafx.media/src/main/native/gstreamer/3rd_party/glib/glib/gbitlock.h ! modules/javafx.media/src/main/native/gstreamer/3rd_party/glib/glib/gbookmarkfile.c ! modules/javafx.media/src/main/native/gstreamer/3rd_party/glib/glib/gbookmarkfile.h ! modules/javafx.media/src/main/native/gstreamer/3rd_party/glib/glib/gbytes.c ! modules/javafx.media/src/main/native/gstreamer/3rd_party/glib/glib/gbytes.h ! modules/javafx.media/src/main/native/gstreamer/3rd_party/glib/glib/gcharset.c ! modules/javafx.media/src/main/native/gstreamer/3rd_party/glib/glib/gcharset.h ! modules/javafx.media/src/main/native/gstreamer/3rd_party/glib/glib/gchecksum.c ! modules/javafx.media/src/main/native/gstreamer/3rd_party/glib/glib/gconstructor.h ! modules/javafx.media/src/main/native/gstreamer/3rd_party/glib/glib/gconvert.c ! modules/javafx.media/src/main/native/gstreamer/3rd_party/glib/glib/gconvert.h ! modules/javafx.media/src/main/native/gstreamer/3rd_party/glib/glib/gdataset.c ! modules/javafx.media/src/main/native/gstreamer/3rd_party/glib/glib/gdataset.h ! modules/javafx.media/src/main/native/gstreamer/3rd_party/glib/glib/gdatasetprivate.h ! modules/javafx.media/src/main/native/gstreamer/3rd_party/glib/glib/gdate.c ! modules/javafx.media/src/main/native/gstreamer/3rd_party/glib/glib/gdate.h ! modules/javafx.media/src/main/native/gstreamer/3rd_party/glib/glib/gdatetime.c ! modules/javafx.media/src/main/native/gstreamer/3rd_party/glib/glib/gdatetime.h ! modules/javafx.media/src/main/native/gstreamer/3rd_party/glib/glib/gdir.c ! modules/javafx.media/src/main/native/gstreamer/3rd_party/glib/glib/gdir.h ! modules/javafx.media/src/main/native/gstreamer/3rd_party/glib/glib/genviron.c ! modules/javafx.media/src/main/native/gstreamer/3rd_party/glib/glib/gerror.c ! modules/javafx.media/src/main/native/gstreamer/3rd_party/glib/glib/gerror.h ! modules/javafx.media/src/main/native/gstreamer/3rd_party/glib/glib/gfileutils.c ! modules/javafx.media/src/main/native/gstreamer/3rd_party/glib/glib/gfileutils.h ! modules/javafx.media/src/main/native/gstreamer/3rd_party/glib/glib/ggettext.c ! modules/javafx.media/src/main/native/gstreamer/3rd_party/glib/glib/ghash.c ! modules/javafx.media/src/main/native/gstreamer/3rd_party/glib/glib/ghash.h ! modules/javafx.media/src/main/native/gstreamer/3rd_party/glib/glib/ghmac.c ! modules/javafx.media/src/main/native/gstreamer/3rd_party/glib/glib/ghook.c ! modules/javafx.media/src/main/native/gstreamer/3rd_party/glib/glib/ghook.h ! modules/javafx.media/src/main/native/gstreamer/3rd_party/glib/glib/ghostutils.c ! modules/javafx.media/src/main/native/gstreamer/3rd_party/glib/glib/giochannel.c ! modules/javafx.media/src/main/native/gstreamer/3rd_party/glib/glib/giochannel.h !
Re: [Approved] RFR: 8193445: JavaFX CSS is applied redundantly leading to significant performance degradation
On Tue, 26 Nov 2019 09:22:01 GMT, Ajit Ghaisas wrote: > The pull request has been updated with additional changes. > > > > Added commits: > - 2054da4c: Address review comments on test > - 4dade6e5: Simpler fix + System test corrections > - bd4a306a: Revert commit1 and commit 2 > > Changes: > - all: https://git.openjdk.java.net/jfx/pull/34/files > - new: https://git.openjdk.java.net/jfx/pull/34/files/12ea8220..2054da4c > > Webrevs: > - full: https://webrevs.openjdk.java.net/jfx/34/webrev.01 > - incr: https://webrevs.openjdk.java.net/jfx/34/webrev.00-01 > > Issue: https://bugs.openjdk.java.net/browse/JDK-8193445 > Stats: 253 lines in 6 files changed: 132 ins; 104 del; 17 mod > Patch: https://git.openjdk.java.net/jfx/pull/34.diff > Fetch: git fetch https://git.openjdk.java.net/jfx pull/34/head:pull/34 Looks good. Approved by kcr (Lead). PR: https://git.openjdk.java.net/jfx/pull/34
Re: RFR: 8130738: TextFlow's tab width is static
On Tue, 26 Nov 2019 18:40:10 GMT, Scott Palmer wrote: > On Thu, 7 Nov 2019 14:56:58 GMT, Kevin Rushforth wrote: > >> On Wed, 6 Nov 2019 19:11:48 GMT, Scott Palmer wrote: >> >>> Added tabSize property to Text and TextFlow and -fx-tab-size CSS attribute >>> to both. TextFlow's tab size overrides that of contained Text nodes. >>> >>> >>> >>> Commits: >>> - 68d221c7: 8130738: TextFlow's tab width is static >>> >>> Changes: https://git.openjdk.java.net/jfx/pull/32/files >>> Webrev: https://webrevs.openjdk.java.net/jfx/32/webrev.00 >>> Issue: https://bugs.openjdk.java.net/browse/JDK-8130738 >>> Stats: 324 lines in 8 files changed: 260 ins; 0 del; 64 mod >>> Patch: https://git.openjdk.java.net/jfx/pull/32.diff >>> Fetch: git fetch https://git.openjdk.java.net/jfx pull/32/head:pull/32 >> >>> I should clamp so it reads back as 1 - Fixed. >> >> This has implications for binding, etc., so I suspect this is not what we >> want. I'll look into this more when I review it. >> >> NOTE: This will need a CSR in addition to two reviewers. You can file the >> CSR any time, but I suggest leaving it in Draft state until Phil and I have >> had a first pass look at the API. > > The indenting was wrong in that section and I had to make some minor changes, > so I corrected the bad indenting. Kevin indicated, "In your specific case, > reformatting the methods in the StyleableProperties nested class that are > adjacent to code you add or modify seems fine, as long as you only change the > indentation and not the line wrapping." > > I didn't intend to trigger an update to this pull request...I find Git > awkward to work with (Mercurial makes more sense to me) so I'm making > mistakes as I bumble around. > > I do expect an update to address the way I've clamped to 1. I agree with > Kevin that it is probable wrong. Just waiting for feedback. I'll take a preliminary look today. I also plan to change the bug title to make it more clear that it is an enhancement request, since it currently reads like a bug report (I'll also change the CSR and PR title to match). PR: https://git.openjdk.java.net/jfx/pull/32
Re: RFR: 8130738: TextFlow's tab width is static
On Thu, 7 Nov 2019 14:56:58 GMT, Kevin Rushforth wrote: > On Wed, 6 Nov 2019 19:11:48 GMT, Scott Palmer wrote: > >> Added tabSize property to Text and TextFlow and -fx-tab-size CSS attribute >> to both. TextFlow's tab size overrides that of contained Text nodes. >> >> >> >> Commits: >> - 68d221c7: 8130738: TextFlow's tab width is static >> >> Changes: https://git.openjdk.java.net/jfx/pull/32/files >> Webrev: https://webrevs.openjdk.java.net/jfx/32/webrev.00 >> Issue: https://bugs.openjdk.java.net/browse/JDK-8130738 >> Stats: 324 lines in 8 files changed: 260 ins; 0 del; 64 mod >> Patch: https://git.openjdk.java.net/jfx/pull/32.diff >> Fetch: git fetch https://git.openjdk.java.net/jfx pull/32/head:pull/32 > >> I should clamp so it reads back as 1 - Fixed. > > This has implications for binding, etc., so I suspect this is not what we > want. I'll look into this more when I review it. > > NOTE: This will need a CSR in addition to two reviewers. You can file the CSR > any time, but I suggest leaving it in Draft state until Phil and I have had a > first pass look at the API. The indenting was wrong in that section and I had to make some minor changes, so I corrected the bad indenting. Kevin indicated, "In your specific case, reformatting the methods in the StyleableProperties nested class that are adjacent to code you add or modify seems fine, as long as you only change the indentation and not the line wrapping." I didn't intend to trigger an update to this pull request...I find Git awkward to work with (Mercurial makes more sense to me) so I'm making mistakes as I bumble around. I do expect an update to address the way I've clamped to 1. I agree with Kevin that it is probable wrong. Just waiting for feedback. PR: https://git.openjdk.java.net/jfx/pull/32
Re: [Rev 02] RFR: 8130738: TextFlow's tab width is static
On Tue, 26 Nov 2019 17:26:33 GMT, Scott Palmer wrote: > The pull request has been updated with a complete new set of changes > (possibly due to a rebase). > > > > Commits: > - 254c40de: Merge remote-tracking branch 'upstream/master' > - a670c4f8: 8130738: TextFlow's tab width is static > - 68d221c7: 8130738: TextFlow's tab width is static > > Changes: https://git.openjdk.java.net/jfx/pull/32/files > Webrev: https://webrevs.openjdk.java.net/jfx/32/webrev.02 > Issue: https://bugs.openjdk.java.net/browse/JDK-8130738 > Stats: 325 lines in 8 files changed: 261 ins; 0 del; 64 mod > Patch: https://git.openjdk.java.net/jfx/pull/32.diff > Fetch: git fetch https://git.openjdk.java.net/jfx pull/32/head:pull/32 Why do we have a ton of extraneous white space changes in the Stylesheet Handling sections of Text.java and TextFlow.java ? PR: https://git.openjdk.java.net/jfx/pull/32
Re: [Rev 02] RFR: 8130738: TextFlow's tab width is static
The pull request has been updated with a complete new set of changes (possibly due to a rebase). Commits: - 254c40de: Merge remote-tracking branch 'upstream/master' - a670c4f8: 8130738: TextFlow's tab width is static - 68d221c7: 8130738: TextFlow's tab width is static Changes: https://git.openjdk.java.net/jfx/pull/32/files Webrev: https://webrevs.openjdk.java.net/jfx/32/webrev.02 Issue: https://bugs.openjdk.java.net/browse/JDK-8130738 Stats: 325 lines in 8 files changed: 261 ins; 0 del; 64 mod Patch: https://git.openjdk.java.net/jfx/pull/32.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/32/head:pull/32 PR: https://git.openjdk.java.net/jfx/pull/32
Re: RFR: 8196587: Remove use of deprecated finalize method from JPEGImageLoader
On Tue, 26 Nov 2019 15:16:38 GMT, Ambarish Rapte wrote:
> The finalize() method is deprecated in JDK9. See [Java 9 deprecated
> features](https://www.oracle.com/technetwork/java/javase/9-deprecated-features-3745636.html).
> And so the JPEGImageLoader.finalize() method should be removed.
>
> The change is,
> 1. Remove finalize method from JPEGImageLoader class.
>
> 2. Instance of JPEGImageLoader is created and used in ImageStorage class.
> JPEGImageLoader.dispose() should be called after it's use over. This would be
> a common call for the other (GIF, PNG, BMP) ImageLoader classes, which have
> empty dispose() method.
>
> 3. JPEGImageLoader.load() method almost always calls the dispose() method
> after the image is loaded. In normal scenario JPEGImageLoader is disposed
> here. The calls to dispose() added in ImageStorage seem logically correct
> place to add and should be added.
>
> Verification:
> Verified :graphics:test and system tests on all three platforms.
> Verified that JPEGImageLoader.dispose() is always initiated by
> JPEGImageLoader.load()
>
>
>
> Commits:
> - b48c8087: 8196587: Remove use of deprecated finalize method from
> JPEGImageLoader
>
> Changes: https://git.openjdk.java.net/jfx/pull/50/files
> Webrev: https://webrevs.openjdk.java.net/jfx/50/webrev.00
> Issue: https://bugs.openjdk.java.net/browse/JDK-8196587
> Stats: 28 lines in 2 files changed: 14 ins; 12 del; 2 mod
> Patch: https://git.openjdk.java.net/jfx/pull/50.diff
> Fetch: git fetch https://git.openjdk.java.net/jfx pull/50/head:pull/50
I still need to double-check all calls to dispose, but I think this is
essentially the right solution, and is ready to be submitted for review. I
added one comment inline.
modules/javafx.graphics/src/main/java/com/sun/javafx/iio/ImageStorage.java line
273:
> 272: } else {
> 273: throw new ImageStorageException("No loader for image
> data");
> 274: }
Now that this is moved inside a try/catch, this `ImageStorageException` will
get wrapped in another `ImageStorageException` if it is ever thrown. You
probably want to catch `ImageStorageException` below and re-throw it without
wrapping it.
PR: https://git.openjdk.java.net/jfx/pull/50
RFR: 8196587: Remove use of deprecated finalize method from JPEGImageLoader
The finalize() method is deprecated in JDK9. See [Java 9 deprecated features](https://www.oracle.com/technetwork/java/javase/9-deprecated-features-3745636.html). And so the JPEGImageLoader.finalize() method should be removed. The change is, 1. Remove finalize method from JPEGImageLoader class. 2. Instance of JPEGImageLoader is created and used in ImageStorage class. JPEGImageLoader.dispose() should be called after it's use over. This would be a common call for the other (GIF, PNG, BMP) ImageLoader classes, which have empty dispose() method. 3. JPEGImageLoader.load() method almost always calls the dispose() method after the image is loaded. In normal scenario JPEGImageLoader is disposed here. The calls to dispose() added in ImageStorage seem logically correct place to add and should be added. Verification: Verified :graphics:test and system tests on all three platforms. Verified that JPEGImageLoader.dispose() is always initiated by JPEGImageLoader.load() Commits: - b48c8087: 8196587: Remove use of deprecated finalize method from JPEGImageLoader Changes: https://git.openjdk.java.net/jfx/pull/50/files Webrev: https://webrevs.openjdk.java.net/jfx/50/webrev.00 Issue: https://bugs.openjdk.java.net/browse/JDK-8196587 Stats: 28 lines in 2 files changed: 14 ins; 12 del; 2 mod Patch: https://git.openjdk.java.net/jfx/pull/50.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/50/head:pull/50 PR: https://git.openjdk.java.net/jfx/pull/50
Re: RFR: 8193445: JavaFX CSS is applied redundantly leading to significant performance degradation
On Tue, 26 Nov 2019 09:22:04 GMT, Kevin Rushforth wrote:
> On Tue, 12 Nov 2019 16:45:04 GMT, Ajit Ghaisas wrote:
>
>> **Issue :**
>> https://bugs.openjdk.java.net/browse/JDK-8193445
>>
>> **Background :**
>> The CSS performance improvement done in
>> [JDK-8151756](https://bugs.openjdk.java.net/browse/JDK-8151756) had to be
>> backed out due to functional regressions reported in
>> [JDK-8185709](https://bugs.openjdk.java.net/browse/JDK-8185709),
>> [JDK-8183100](https://bugs.openjdk.java.net/browse/JDK-8183100) and
>> [JDK-8168951](https://bugs.openjdk.java.net/browse/JDK-8168951).
>> Refer to [JDK-8183100](https://bugs.openjdk.java.net/browse/JDK-8183100) for
>> more details on this backout.
>>
>> **Description :**
>> This PR reintroduces the CSS performance improvement fix done in
>> [JDK-8151756](https://bugs.openjdk.java.net/browse/JDK-8151756) while
>> addressing the functional regressions that were reported in
>> [JDK-8185709](https://bugs.openjdk.java.net/browse/JDK-8185709),
>> [JDK-8183100](https://bugs.openjdk.java.net/browse/JDK-8183100) and
>> [JDK-8168951](https://bugs.openjdk.java.net/browse/JDK-8168951).
>> For ease of review, I have made two separate commits -
>> 1) [Commit
>> 1](https://github.com/openjdk/jfx/pull/34/commits/d964675ebc2a42f2fd6928b773819502683f2334)
>> - Reintroduces the CSS performance improvement fix done in
>> [JDK-8151756](https://bugs.openjdk.java.net/browse/JDK-8151756) - most of
>> the patch applied cleanly.
>> 2) [Commit 2
>> ](https://github.com/openjdk/jfx/pull/34/commits/12ea8220a730ff8d98d520ce870691d54f0de00f)-
>> fixes the functional regressions keeping performance improvement intact +
>> adds a system test
>>
>> **Root Cause :**
>> CSS performance improvement fix proposed in [JDK-8151756
>> ](https://bugs.openjdk.java.net/browse/JDK-8151756)correctly avoids the
>> redundant CSS reapplication to children of a Parent.
>> What was missed earlier in [JDK-8151756
>> ](https://bugs.openjdk.java.net/browse/JDK-8151756) fix : "CSS reapplication
>> to the Parent itself”.
>> This missing piece was the root cause of all functional regressions reported
>> against [JDK-8151756](https://bugs.openjdk.java.net/browse/JDK-8151756)
>>
>> **Fix :**
>> Fixed the identified root cause. See commit 2.
>>
>> **Testing :**
>> 1. All passing unit tests continue to pass
>> 2. New system test (based on
>> [JDK-8209830](https://bugs.openjdk.java.net/browse/JDK-8209830)) added in
>> this PR - fails before this fix and passes after the fix
>> 3. System test JDK8183100Test continues to pass
>> 4. All test cases attached to regressions
>> [JDK-8185709](https://bugs.openjdk.java.net/browse/JDK-8185709),
>> [JDK-8183100](https://bugs.openjdk.java.net/browse/JDK-8183100) and
>> [JDK-8168951](https://bugs.openjdk.java.net/browse/JDK-8168951) pass with
>> this fix
>>
>> In addition, testing by community with specific CSS performance /
>> functionality will be helpful.
>>
>>
>>
>> Commits:
>> - 12ea8220: Fix for functional regressions of JDK-8151756 + add a sytem test
>> - d964675e: Reintroduce JDK-8151756 CSS performance fix
>>
>> Changes: https://git.openjdk.java.net/jfx/pull/34/files
>> Webrev: https://webrevs.openjdk.java.net/jfx/34/webrev.00
>> Issue: https://bugs.openjdk.java.net/browse/JDK-8193445
>> Stats: 121 lines in 5 files changed: 104 ins; 0 del; 17 mod
>> Patch: https://git.openjdk.java.net/jfx/pull/34.diff
>> Fetch: git fetch https://git.openjdk.java.net/jfx pull/34/head:pull/34
>
> The fix itself looks good and is a much safer approach than the previous one.
> I've done a fair bit of testing and can see no regressions as a result of
> this fix. I did find one unrelated issue while testing (a visual bug
> introduced back in JDK 10) that I will file separately.
>
> The test is pretty close, but still needs a few changes. The main problem is
> that it does not catch the performance problem, meaning if you run it without
> the fix, it will still pass. Your test class extends
> `javafx.application.Application`, which can cause problems, since JUnit will
> create a new instance of the test class that is different from the one
> created by the call to `Application.launch` in the `@BeforeClass` method.
> This in turn leads to the `rootPane` instance used by the test method being
> different from the one used as the root of the visible scene. The fix is to
> use a separate nested (static) subclass of Application, and make the rootPane
> field a static field. I have left inline comments for this and a few other
> things I noticed.
>
> tests/system/src/test/java/test/javafx/scene/QuadraticCssTimeTest.java line
> 63:
>
>> 62:
>> 63: public class QuadraticCssTimeTest extends Application {
>> 64:
>
> Remove `extends Application`
>
> tests/system/src/test/java/test/javafx/scene/QuadraticCssTimeTest.java line
> 66:
>
>> 65: static private CountDownLatch startupLatch;
>> 66: static private
Re: RFR: 8200224: Multiple press event when JFXPanel gains focus
On Tue, 26 Nov 2019 13:06:38 GMT, Florian Kirmaier
wrote:
> On Tue, 26 Nov 2019 09:24:01 GMT, Kevin Rushforth wrote:
>
>> On Tue, 29 Oct 2019 11:27:35 GMT, Florian Kirmaier
>> wrote:
>>
>>> Original PR: https://github.com/javafxports/openjdk-jfx/pull/591
>>>
>>> A side effect of JDK-8200224 is, that the first click on a JFXPanel is
>>> always a double click.
>>> The cause for JDK-8200224 is the fix for JDK-8087914 (Clicking on Menu in
>>> JFXPanel ignored if another swing component has focus).
>>> This fix introduced synthesized press-events, which is an unsafe fix for
>>> the problem.
>>>
>>> The pull request introduces a new fix for the problem.
>>> Instead of simulating new input-events, we set JavaFX Scene/Window to
>>> focused.
>>> We do so, by using setFocused.
>>> When the original Swing-Focus event is called slightly later, it won't have
>>> any side-effects,
>>> because calling setFocused just sets the value of a boolean property.
>>>
>>> I've now also added a unit-test for the problem.
>>>
>>>
>>>
>>> Commits:
>>> - 314e423c: JDK-8200224
>>> - 725e5fef: JDK-8200224
>>>
>>> Changes: https://git.openjdk.java.net/jfx/pull/25/files
>>> Webrev: https://webrevs.openjdk.java.net/jfx/25/webrev.00
>>> Issue: https://bugs.openjdk.java.net/browse/JDK-8200224
>>> Stats: 140 lines in 2 files changed: 133 ins; 2 del; 5 mod
>>> Patch: https://git.openjdk.java.net/jfx/pull/25.diff
>>> Fetch: git fetch https://git.openjdk.java.net/jfx pull/25/head:pull/25
>>
>> I left a couple additional comments about the test changes. Namely:
>>
>> 1. You didn't fully revert the changes to `SwingFXUtilsTest`
>> 2. Your new test should be put in its own class in `test.javafx.embed.swing`
>> (and not in the exist mac-only Robot test)
>>
>> tests/system/src/test/java/test/javafx/embed/swing/SwingFXUtilsTest.java
>> line 87:
>>
>>> 86: testFromFXImg("opaque.gif");
>>> 87: testFromFXImg("opaque.jpg");
>>> 88: testFromFXImg("opaque.png");
>>
>> You didn't fully revert your change to this file. The above needs to be
>> restored such that when you are done, this file is unchanged versus master,
>> and not part of the PR.
>>
>> tests/system/src/test/java/test/robot/javafx/embed/swing/JFXPanelTest.java
>> line 25:
>>
>>> 24: */
>>> 25: package test.robot.javafx.embed.swing;
>>> 26:
>>
>> Since you aren't using `Robot` I'll repeat my earlier recommendation to put
>> your new test in `test.javafx.embed.swing` (as a new test class) rather than
>> modifying this existing test class. This class isn't suitable anyway, since
>> it is only run on Mac.
>>
>> tests/system/src/test/java/test/robot/javafx/embed/swing/JFXPanelTest.java
>> line 40:
>>
>>> 39: import javax.swing.*;
>>> 40: import java.awt.*;
>>> 41: import java.awt.event.InputEvent;
>>
>> The use of wild card imports is discouraged (except for static imports).
>>
>> tests/system/src/test/java/test/robot/javafx/embed/swing/JFXPanelTest.java
>> line 129:
>>
>>> 128: JPanel jpanel = new JPanel();
>>> 129: jpanel.add(fxPnl);
>>> 130: jframe.setContentPane(jpanel);
>>
>> You didn't add an initial dummy `JFXPanel` so I suspect it will still not
>> catch the bug (meaning it will still pass even without your patch).
>>
>>
>>
>> Changes requested by kcr (Lead).
>
> 1. I've restored the test. But the git history is now very chaotic.
> Especially the moves might cause problems. Do the commits get squashed after
> merging? Otherwise, it might make sense, to redo the changes on a fresh
> branch and create a new pull request.
> 2. The test works now on windows. : )
Yes, the commits are squashed, so don't worry about the history (in worst case
I would ask you to do a squash-rebase / force push rather than create a new PR,
but that isn't needed here).
At first glance the changes you made look good. I'll take a closer look later.
@prsadhuk please also review this.
PR: https://git.openjdk.java.net/jfx/pull/25
Re: [Rev 04] RFR: 8200224: Multiple press event when JFXPanel gains focus
The pull request has been updated with additional changes. Added commits: - 24385eb8: JDK-8200224 - e0829ad3: JDK-8200224 - c190384f: JDK-8200224 - 17b458b1: JDK-8200224 Changes: - all: https://git.openjdk.java.net/jfx/pull/25/files - new: https://git.openjdk.java.net/jfx/pull/25/files/44774dfb..24385eb8 Webrevs: - full: https://webrevs.openjdk.java.net/jfx/25/webrev.04 - incr: https://webrevs.openjdk.java.net/jfx/25/webrev.03-04 Issue: https://bugs.openjdk.java.net/browse/JDK-8200224 Stats: 141 lines in 2 files changed: 123 ins; 11 del; 7 mod Patch: https://git.openjdk.java.net/jfx/pull/25.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/25/head:pull/25 PR: https://git.openjdk.java.net/jfx/pull/25
Re: RFR: 8200224: Multiple press event when JFXPanel gains focus
On Tue, 26 Nov 2019 09:24:01 GMT, Kevin Rushforth wrote:
> On Tue, 29 Oct 2019 11:27:35 GMT, Florian Kirmaier
> wrote:
>
>> Original PR: https://github.com/javafxports/openjdk-jfx/pull/591
>>
>> A side effect of JDK-8200224 is, that the first click on a JFXPanel is
>> always a double click.
>> The cause for JDK-8200224 is the fix for JDK-8087914 (Clicking on Menu in
>> JFXPanel ignored if another swing component has focus).
>> This fix introduced synthesized press-events, which is an unsafe fix for the
>> problem.
>>
>> The pull request introduces a new fix for the problem.
>> Instead of simulating new input-events, we set JavaFX Scene/Window to
>> focused.
>> We do so, by using setFocused.
>> When the original Swing-Focus event is called slightly later, it won't have
>> any side-effects,
>> because calling setFocused just sets the value of a boolean property.
>>
>> I've now also added a unit-test for the problem.
>>
>>
>>
>> Commits:
>> - 314e423c: JDK-8200224
>> - 725e5fef: JDK-8200224
>>
>> Changes: https://git.openjdk.java.net/jfx/pull/25/files
>> Webrev: https://webrevs.openjdk.java.net/jfx/25/webrev.00
>> Issue: https://bugs.openjdk.java.net/browse/JDK-8200224
>> Stats: 140 lines in 2 files changed: 133 ins; 2 del; 5 mod
>> Patch: https://git.openjdk.java.net/jfx/pull/25.diff
>> Fetch: git fetch https://git.openjdk.java.net/jfx pull/25/head:pull/25
>
> I left a couple additional comments about the test changes. Namely:
>
> 1. You didn't fully revert the changes to `SwingFXUtilsTest`
> 2. Your new test should be put in its own class in `test.javafx.embed.swing`
> (and not in the exist mac-only Robot test)
>
> tests/system/src/test/java/test/javafx/embed/swing/SwingFXUtilsTest.java line
> 87:
>
>> 86: testFromFXImg("opaque.gif");
>> 87: testFromFXImg("opaque.jpg");
>> 88: testFromFXImg("opaque.png");
>
> You didn't fully revert your change to this file. The above needs to be
> restored such that when you are done, this file is unchanged versus master,
> and not part of the PR.
>
> tests/system/src/test/java/test/robot/javafx/embed/swing/JFXPanelTest.java
> line 25:
>
>> 24: */
>> 25: package test.robot.javafx.embed.swing;
>> 26:
>
> Since you aren't using `Robot` I'll repeat my earlier recommendation to put
> your new test in `test.javafx.embed.swing` (as a new test class) rather than
> modifying this existing test class. This class isn't suitable anyway, since
> it is only run on Mac.
>
> tests/system/src/test/java/test/robot/javafx/embed/swing/JFXPanelTest.java
> line 40:
>
>> 39: import javax.swing.*;
>> 40: import java.awt.*;
>> 41: import java.awt.event.InputEvent;
>
> The use of wild card imports is discouraged (except for static imports).
>
> tests/system/src/test/java/test/robot/javafx/embed/swing/JFXPanelTest.java
> line 129:
>
>> 128: JPanel jpanel = new JPanel();
>> 129: jpanel.add(fxPnl);
>> 130: jframe.setContentPane(jpanel);
>
> You didn't add an initial dummy `JFXPanel` so I suspect it will still not
> catch the bug (meaning it will still pass even without your patch).
>
>
>
> Changes requested by kcr (Lead).
1. I've restored the test. But the git history is now very chaotic. Especially
the moves might cause problems. Do the commits get squashed after merging?
Otherwise, it might make sense, to redo the changes on a fresh branch and
create a new pull request.
2. The test works now on windows. : )
PR: https://git.openjdk.java.net/jfx/pull/25
Re: [Rev 03] RFR: 8200224: Multiple press event when JFXPanel gains focus
On Tue, 26 Nov 2019 09:24:01 GMT, Kevin Rushforth wrote:
> On Tue, 26 Nov 2019 09:23:58 GMT, Florian Kirmaier
> wrote:
>
>> The pull request has been updated with a complete new set of changes
>> (possibly due to a rebase).
>>
>>
>>
>> Commits:
>> - 44774dfb: JDK-8200224
>> - d1309fb6: JDK-8200224
>> - 53a66b8f: JDK-8200224
>> - 0be3cb5b: JDK-8200224
>> - 03b59288: Merge branch 'master' of https://github.com/openjdk/jfx into
>> JDK-8200224
>> - 5cd96a56: JDK-8200224
>> - 85c87628: JDK-8200224
>> - 314e423c: JDK-8200224
>> - 725e5fef: JDK-8200224
>>
>> Changes: https://git.openjdk.java.net/jfx/pull/25/files
>> Webrev: https://webrevs.openjdk.java.net/jfx/25/webrev.03
>> Issue: https://bugs.openjdk.java.net/browse/JDK-8200224
>> Stats: 272 lines in 3 files changed: 147 ins; 120 del; 5 mod
>> Patch: https://git.openjdk.java.net/jfx/pull/25.diff
>> Fetch: git fetch https://git.openjdk.java.net/jfx pull/25/head:pull/25
>
> To follow-up on my previous comment here are the requested changes:
>
> 1. Restore `
> tests/system/src/test/java/test/robot/javafx/embed/swing/JFXPanelTest.java`,
> which was removed by your last commit, so that it doesn't show up in the PR.
> 2. In the new test, add a dummy `JFXPanel` to the `JPanel` so that the test
> `JFXPanel` is the second one added (see my inline comments). This will allow
> the test to catch the error on Windows by failing without your fix.
> 3. A few other inline comments on the new test.
>
> tests/system/src/test/java/test/javafx/embed/swing/JFXPanelTest.java line 2:
>
>> 1: /*
>> 2: * Copyright (c) 2017, Oracle and/or its affiliates. All rights reserved.
>> 3: * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>
> That should be `2019`
>
> tests/system/src/test/java/test/javafx/embed/swing/JFXPanelTest.java line 35:
>
>> 34:
>> 35:
>> 36: import javax.swing.JFrame;
>
> You only need one blank line here.
>
> tests/system/src/test/java/test/javafx/embed/swing/JFXPanelTest.java line 94:
>
>> 93:
>> 94: class TestFXPanel extends JFXPanel {
>> 95: protected void processMouseEventPublic(MouseEvent e) {
>
> This class can be `static`
>
> tests/system/src/test/java/test/javafx/embed/swing/JFXPanelTest.java line 108:
>
>> 107:
>> 108: SwingUtilities.invokeLater(() -> {
>> 109: TestFXPanel fxPnl = new TestFXPanel();
>
> Add the following here:
> JFXPanel dummyFXPanel = new JFXPanel();
> dummyFXPanel.setPreferredSize(new Dimension(100, 100));
>
> tests/system/src/test/java/test/javafx/embed/swing/JFXPanelTest.java line 112:
>
>> 111: JFrame jframe = new JFrame();
>> 112: JPanel jpanel = new JPanel();
>> 113: jpanel.add(fxPnl);
>
> Add the following here:
> jpanel.add(dummyFXPanel);
>
> tests/system/src/test/java/test/javafx/embed/swing/JFXPanelTest.java line 118:
>
>> 117:
>> 118: Platform.runLater(() -> {
>> 119: Group grp = new Group();
>
> Add the following here:
> Scene dummyScene = new Scene(new Group());
> dummyFXPanel.setScene(dummyScene);
>
> tests/system/src/test/java/test/javafx/embed/swing/JFXPanelTest.java line 138:
>
>> 137:
>> 138: if(!firstPressedEventLatch.await(5000, TimeUnit.MILLISECONDS)) {
>> 139: throw new Exception();
>
> This `if` test can be written more succinctly as:
>
> assertTrue(firstPressedEventLatch.await(5000, TimeUnit.MILLISECONDS));
>
> tests/system/src/test/java/test/javafx/embed/swing/JFXPanelTest.java line 72:
>
>> 71: @BeforeClass
>> 72: public static void doSetupOnce() {
>> 73: // Start the Application
>
> If you add `throws Exception` then you don't need the try/catch around the
> `launchLatch.await`.
>
> tests/system/src/test/java/test/javafx/embed/swing/JFXPanelTest.java line 119:
>
>> 118: Platform.runLater(() -> {
>> 119: Group grp = new Group();
>> 120: Scene scene = new Scene(new Group());
>
> This variable is unused.
>
>
>
> Changes requested by kcr (Lead).
The if is triggered, when the time runs out.
remvoed
done
done
It's now simplified.
done
PR: https://git.openjdk.java.net/jfx/pull/25
Re: RFR: 8200224: Multiple press event when JFXPanel gains focus
On Tue, 29 Oct 2019 11:27:35 GMT, Florian Kirmaier
wrote:
> Original PR: https://github.com/javafxports/openjdk-jfx/pull/591
>
> A side effect of JDK-8200224 is, that the first click on a JFXPanel is always
> a double click.
> The cause for JDK-8200224 is the fix for JDK-8087914 (Clicking on Menu in
> JFXPanel ignored if another swing component has focus).
> This fix introduced synthesized press-events, which is an unsafe fix for the
> problem.
>
> The pull request introduces a new fix for the problem.
> Instead of simulating new input-events, we set JavaFX Scene/Window to focused.
> We do so, by using setFocused.
> When the original Swing-Focus event is called slightly later, it won't have
> any side-effects,
> because calling setFocused just sets the value of a boolean property.
>
> I've now also added a unit-test for the problem.
>
>
>
> Commits:
> - 314e423c: JDK-8200224
> - 725e5fef: JDK-8200224
>
> Changes: https://git.openjdk.java.net/jfx/pull/25/files
> Webrev: https://webrevs.openjdk.java.net/jfx/25/webrev.00
> Issue: https://bugs.openjdk.java.net/browse/JDK-8200224
> Stats: 140 lines in 2 files changed: 133 ins; 2 del; 5 mod
> Patch: https://git.openjdk.java.net/jfx/pull/25.diff
> Fetch: git fetch https://git.openjdk.java.net/jfx pull/25/head:pull/25
I left a couple additional comments about the test changes. Namely:
1. You didn't fully revert the changes to `SwingFXUtilsTest`
2. Your new test should be put in its own class in `test.javafx.embed.swing`
(and not in the exist mac-only Robot test)
tests/system/src/test/java/test/javafx/embed/swing/SwingFXUtilsTest.java line
87:
> 86: testFromFXImg("opaque.gif");
> 87: testFromFXImg("opaque.jpg");
> 88: testFromFXImg("opaque.png");
You didn't fully revert your change to this file. The above needs to be
restored such that when you are done, this file is unchanged versus master, and
not part of the PR.
tests/system/src/test/java/test/robot/javafx/embed/swing/JFXPanelTest.java line
25:
> 24: */
> 25: package test.robot.javafx.embed.swing;
> 26:
Since you aren't using `Robot` I'll repeat my earlier recommendation to put
your new test in `test.javafx.embed.swing` (as a new test class) rather than
modifying this existing test class. This class isn't suitable anyway, since it
is only run on Mac.
tests/system/src/test/java/test/robot/javafx/embed/swing/JFXPanelTest.java line
40:
> 39: import javax.swing.*;
> 40: import java.awt.*;
> 41: import java.awt.event.InputEvent;
The use of wild card imports is discouraged (except for static imports).
tests/system/src/test/java/test/robot/javafx/embed/swing/JFXPanelTest.java line
129:
> 128: JPanel jpanel = new JPanel();
> 129: jpanel.add(fxPnl);
> 130: jframe.setContentPane(jpanel);
You didn't add an initial dummy `JFXPanel` so I suspect it will still not catch
the bug (meaning it will still pass even without your patch).
Changes requested by kcr (Lead).
PR: https://git.openjdk.java.net/jfx/pull/25
Re: RFR: 8200224: Multiple press event when JFXPanel gains focus
On Tue, 26 Nov 2019 09:23:59 GMT, Florian Kirmaier
wrote:
> On Wed, 13 Nov 2019 22:11:14 GMT, Kevin Rushforth wrote:
>
>> On Wed, 13 Nov 2019 01:17:44 GMT, Kevin Rushforth wrote:
>>
>>> On Tue, 12 Nov 2019 10:35:36 GMT, Florian Kirmaier
>>> wrote:
>>>
On Wed, 6 Nov 2019 08:30:47 GMT, Kevin Rushforth wrote:
> On Tue, 29 Oct 2019 11:27:35 GMT, Florian Kirmaier
> wrote:
>
>> Original PR: https://github.com/javafxports/openjdk-jfx/pull/591
>>
>> A side effect of JDK-8200224 is, that the first click on a JFXPanel is
>> always a double click.
>> The cause for JDK-8200224 is the fix for JDK-8087914 (Clicking on Menu
>> in JFXPanel ignored if another swing component has focus).
>> This fix introduced synthesized press-events, which is an unsafe fix for
>> the problem.
>>
>> The pull request introduces a new fix for the problem.
>> Instead of simulating new input-events, we set JavaFX Scene/Window to
>> focused.
>> We do so, by using setFocused.
>> When the original Swing-Focus event is called slightly later, it won't
>> have any side-effects,
>> because calling setFocused just sets the value of a boolean property.
>>
>> I've now also added a unit-test for the problem.
>>
>>
>>
>> Commits:
>> - 314e423c: JDK-8200224
>> - 725e5fef: JDK-8200224
>>
>> Changes: https://git.openjdk.java.net/jfx/pull/25/files
>> Webrev: https://webrevs.openjdk.java.net/jfx/25/webrev.00
>> Issue: https://bugs.openjdk.java.net/browse/JDK-8200224
>> Stats: 140 lines in 2 files changed: 133 ins; 2 del; 5 mod
>> Patch: https://git.openjdk.java.net/jfx/pull/25.diff
>> Fetch: git fetch https://git.openjdk.java.net/jfx pull/25/head:pull/25
>
> modules/javafx.swing/src/main/java/javafx/embed/swing/JFXPanel.java line
> 457:
>
>> 456:
>> 457: sendMouseEventToFX(e);
>> 458: super.processMouseEvent(e);
>
> typo: save --> safe
>
> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
> line 2:
>
>> 1: /*
>> 2: * Copyright (c) 2014, 2016 Oracle and/or its affiliates. All rights
>> reserved.
>> 3: * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>
> replace `2014, 2016` with `2019,`
>
> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
> line 28:
>
>> 27:
>> 28: import javafx.application.Application;
>> 29: import javafx.application.Platform;
>
> Remove unused import (several of the below imports are similarly unused).
> Also, since this is a new test, please sort the imports.
>
> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
> line 48:
>
>> 47:
>> 48: import static junit.framework.Assert.assertEquals;
>> 49: import static org.junit.Assert.assertNotNull;
>
> This method should be imported from `org.junit` package.
>
> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
> line 116:
>
>> 115: MouseEvent e = new MouseEvent(fxPnl,
>> MouseEvent.MOUSE_PRESSED, 0, MouseEvent.BUTTON1_DOWN_MASK,
>> 116: 5, 5, 1, false, MouseEvent.BUTTON1);
>> 117:
>
> This doesn't appear to trigger the bug -- at least on Windows. The test
> passes for me with or without your fix. You might consider moving it to
> the system tests project, under `tests/system/src/test/java/test/robot`
> and using `Robot` to effect the mouse press.
>
> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
> line 57:
>
>> 56:
>> 57:
>> 58: @BeforeClass
>
> You can remove the extra blank line.
>
> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
> line 65:
>
>> 64:
>> 65:
>> 66: try {
>
> You can remove the extra blank line.
>
> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
> line 79:
>
>> 78:
>> 79: class TestFXPanel extends JFXPanel {
>> 80: protected void processMouseEventPublic(MouseEvent e) {
>
> I think this can be a static class.
>
> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
> line 88:
>
>> 87:
>> 88: CountDownLatch firstPressedEventLatch = new
>> CountDownLatch(1);
>> 89:
>
> This can be final.
>
> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
> line 91:
>
>> 90: // It's an array, so we can mutate it inside of lambda
>> statement
>> 91: int[] pressedEventCounter = {0};
>> 92:
>
Re: [Rev 03] RFR: 8200224: Multiple press event when JFXPanel gains focus
On Tue, 26 Nov 2019 09:23:58 GMT, Florian Kirmaier
wrote:
> The pull request has been updated with a complete new set of changes
> (possibly due to a rebase).
>
>
>
> Commits:
> - 44774dfb: JDK-8200224
> - d1309fb6: JDK-8200224
> - 53a66b8f: JDK-8200224
> - 0be3cb5b: JDK-8200224
> - 03b59288: Merge branch 'master' of https://github.com/openjdk/jfx into
> JDK-8200224
> - 5cd96a56: JDK-8200224
> - 85c87628: JDK-8200224
> - 314e423c: JDK-8200224
> - 725e5fef: JDK-8200224
>
> Changes: https://git.openjdk.java.net/jfx/pull/25/files
> Webrev: https://webrevs.openjdk.java.net/jfx/25/webrev.03
> Issue: https://bugs.openjdk.java.net/browse/JDK-8200224
> Stats: 272 lines in 3 files changed: 147 ins; 120 del; 5 mod
> Patch: https://git.openjdk.java.net/jfx/pull/25.diff
> Fetch: git fetch https://git.openjdk.java.net/jfx pull/25/head:pull/25
To follow-up on my previous comment here are the requested changes:
1. Restore `
tests/system/src/test/java/test/robot/javafx/embed/swing/JFXPanelTest.java`,
which was removed by your last commit, so that it doesn't show up in the PR.
2. In the new test, add a dummy `JFXPanel` to the `JPanel` so that the test
`JFXPanel` is the second one added (see my inline comments). This will allow
the test to catch the error on Windows by failing without your fix.
3. A few other inline comments on the new test.
tests/system/src/test/java/test/javafx/embed/swing/JFXPanelTest.java line 2:
> 1: /*
> 2: * Copyright (c) 2017, Oracle and/or its affiliates. All rights reserved.
> 3: * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
That should be `2019`
tests/system/src/test/java/test/javafx/embed/swing/JFXPanelTest.java line 35:
> 34:
> 35:
> 36: import javax.swing.JFrame;
You only need one blank line here.
tests/system/src/test/java/test/javafx/embed/swing/JFXPanelTest.java line 94:
> 93:
> 94: class TestFXPanel extends JFXPanel {
> 95: protected void processMouseEventPublic(MouseEvent e) {
This class can be `static`
tests/system/src/test/java/test/javafx/embed/swing/JFXPanelTest.java line 108:
> 107:
> 108: SwingUtilities.invokeLater(() -> {
> 109: TestFXPanel fxPnl = new TestFXPanel();
Add the following here:
JFXPanel dummyFXPanel = new JFXPanel();
dummyFXPanel.setPreferredSize(new Dimension(100, 100));
tests/system/src/test/java/test/javafx/embed/swing/JFXPanelTest.java line 112:
> 111: JFrame jframe = new JFrame();
> 112: JPanel jpanel = new JPanel();
> 113: jpanel.add(fxPnl);
Add the following here:
jpanel.add(dummyFXPanel);
tests/system/src/test/java/test/javafx/embed/swing/JFXPanelTest.java line 118:
> 117:
> 118: Platform.runLater(() -> {
> 119: Group grp = new Group();
Add the following here:
Scene dummyScene = new Scene(new Group());
dummyFXPanel.setScene(dummyScene);
tests/system/src/test/java/test/javafx/embed/swing/JFXPanelTest.java line 138:
> 137:
> 138: if(!firstPressedEventLatch.await(5000, TimeUnit.MILLISECONDS)) {
> 139: throw new Exception();
This `if` test can be written more succinctly as:
assertTrue(firstPressedEventLatch.await(5000, TimeUnit.MILLISECONDS));
tests/system/src/test/java/test/javafx/embed/swing/JFXPanelTest.java line 72:
> 71: @BeforeClass
> 72: public static void doSetupOnce() {
> 73: // Start the Application
If you add `throws Exception` then you don't need the try/catch around the
`launchLatch.await`.
tests/system/src/test/java/test/javafx/embed/swing/JFXPanelTest.java line 119:
> 118: Platform.runLater(() -> {
> 119: Group grp = new Group();
> 120: Scene scene = new Scene(new Group());
This variable is unused.
Changes requested by kcr (Lead).
PR: https://git.openjdk.java.net/jfx/pull/25
Re: [Rev 02] RFR: 8200224: Multiple press event when JFXPanel gains focus
On Wed, 13 Nov 2019 21:53:13 GMT, Kevin Rushforth wrote:
> On Tue, 12 Nov 2019 10:23:16 GMT, Florian Kirmaier
> wrote:
>
>> The pull request has been updated with additional changes.
>>
>>
>>
>> Added commits:
>> - 5cd96a56: JDK-8200224
>>
>> Changes:
>> - all: https://git.openjdk.java.net/jfx/pull/25/files
>> - new: https://git.openjdk.java.net/jfx/pull/25/files/85c87628..5cd96a56
>>
>> Webrevs:
>> - full: https://webrevs.openjdk.java.net/jfx/25/webrev.02
>> - incr: https://webrevs.openjdk.java.net/jfx/25/webrev.01-02
>>
>> Issue: https://bugs.openjdk.java.net/browse/JDK-8200224
>> Stats: 5 lines in 1 file changed: 0 ins; 5 del; 0 mod
>> Patch: https://git.openjdk.java.net/jfx/pull/25.diff
>> Fetch: git fetch https://git.openjdk.java.net/jfx pull/25/head:pull/25
>
> I see that when you made your earlier comment regarding `System.exit`, you
> really meant that the existing swing test was calling `Platform.exit`, which
> isn't the same thing; it does shut down the JavaFX runtime, which is intended.
>
> The problem you are running into is that gradle runs multiple tests in the
> same VM and in an undefined, and unpredicable, order. This means that tests
> need to take care not to alter or rely on global state such as threads,
> static fields, global JavaFX state, and the running (or lack thereof) of the
> JavaFX runtime. The swing tests violate this rule and are therefore
> inherently unstable. The only reason this hasn't been a problem up to now is
> that the javafx.swing module contains a single test class. I will file a new
> test bug to address it -- probably by moving that test to the `systemTests`
> project.
>
> You will need to move your test into the `systemTests` project under the
> `tests/system/` directory. Such tests are valid in the system tests project
> because we run each test in that project in its own VM. Once your proposed
> test is robust (meaning consistently catches the bug without your fix and
> consistently passes with your fix) on all platforms without using Robot, then
> `tests/system/src/test/java/test/javafx/embed/swing/` would be the best place
> to put your new test.
>
> Regarding the test itself, it still doesn't fail for me on Windows without
> your fix. I ran the test program attached to the bug and I see something that
> might help explain this. That test program creates a pair of JFXPanel objects
> and adds both to the JPanel. If I first click on the first one, then it only
> shows a single click. Every time after that, if I click on a new JFXPanel,
> then I get 2 clicks. If, instead, I click on the second JFXPanel right after
> starting the program, I get 2 clicks the first time. With that in mind, I
> modified your test to add a dummy JFXPanel to the JPanel before the one you
> are sending the mouse pressed event to, and then it then does what I expect:
> it catches the bug without your fix and passes with your fix. That might help
> you come up with a more robust test. I added some specific comments on the
> test as well.
>
> modules/javafx.swing/src/test/java/test/javafx/embed/swing/SwingFXUtilsTest.java
> line 46:
>
>> 45:
>> 46:
>> 47: public class SwingFXUtilsTest {
>
> Your proposed fix for this test class is not the right fix, and needs to be
> reverted. It highlights an existing bug with the swing tests, which I will
> address in general comments.
>
> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
> line 100:
>
>> 99: jpanel.add(fxPnl);
>> 100: jframe.setContentPane(jpanel);
>> 101: jframe.setVisible(true);
>
> You should call `jframe.pack()` here.
>
> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
> line 128:
>
>> 127:
>> 128: Thread.sleep(100); // there should be no pressed event after
>> the initial one. Let's wait for 0.1s and check again.
>> 129: assertEquals(1, pressedEventCounter[0]);
>
> I recommend using 500 msec so we don't risk missing a failing click on slower
> systems.
>
> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
> line 60:
>
>> 59: public static void doSetupOnce() {
>> 60: if (setupIsDone) return;
>> 61: Platform.startup(() -> {
>
> This is not needed if you revert the changes to the other test class.
>
> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
> line 130:
>
>> 129: assertEquals(1, pressedEventCounter[0]);
>> 130: }
>> 131: }
>
> You will want to add a cleanup method, annotated with `@AfterClass` to call
> `Platform.exit` and dispose of the JFrame.
>
> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
> line 106:
>
>> 105: Scene scene = new Scene(new Group());
>> 106: scene.getRoot().requestFocus();
>> 107:
>
> I don't think requesting focus is necessary (or desired).
>
>
Re: RFR: 8193445: JavaFX CSS is applied redundantly leading to significant performance degradation
On Tue, 26 Nov 2019 09:22:02 GMT, Ajit Ghaisas wrote: > On Tue, 26 Nov 2019 09:22:01 GMT, David Grieve wrote: > >> On Tue, 26 Nov 2019 09:22:01 GMT, Ajit Ghaisas wrote: >> >>> On Tue, 19 Nov 2019 14:29:16 GMT, David Grieve >>> wrote: >>> On Tue, 19 Nov 2019 10:48:52 GMT, Ajit Ghaisas wrote: > On Fri, 15 Nov 2019 09:14:04 GMT, Ajit Ghaisas > wrote: > >> On Thu, 14 Nov 2019 18:33:05 GMT, Kevin Rushforth >> wrote: >> >>> On Tue, 12 Nov 2019 16:45:04 GMT, Ajit Ghaisas >>> wrote: >>> **Issue :** https://bugs.openjdk.java.net/browse/JDK-8193445 **Background :** The CSS performance improvement done in [JDK-8151756](https://bugs.openjdk.java.net/browse/JDK-8151756) had to be backed out due to functional regressions reported in [JDK-8185709](https://bugs.openjdk.java.net/browse/JDK-8185709), [JDK-8183100](https://bugs.openjdk.java.net/browse/JDK-8183100) and [JDK-8168951](https://bugs.openjdk.java.net/browse/JDK-8168951). Refer to [JDK-8183100](https://bugs.openjdk.java.net/browse/JDK-8183100) for more details on this backout. **Description :** This PR reintroduces the CSS performance improvement fix done in [JDK-8151756](https://bugs.openjdk.java.net/browse/JDK-8151756) while addressing the functional regressions that were reported in [JDK-8185709](https://bugs.openjdk.java.net/browse/JDK-8185709), [JDK-8183100](https://bugs.openjdk.java.net/browse/JDK-8183100) and [JDK-8168951](https://bugs.openjdk.java.net/browse/JDK-8168951). For ease of review, I have made two separate commits - 1) [Commit 1](https://github.com/openjdk/jfx/pull/34/commits/d964675ebc2a42f2fd6928b773819502683f2334) - Reintroduces the CSS performance improvement fix done in [JDK-8151756](https://bugs.openjdk.java.net/browse/JDK-8151756) - most of the patch applied cleanly. 2) [Commit 2 ](https://github.com/openjdk/jfx/pull/34/commits/12ea8220a730ff8d98d520ce870691d54f0de00f)- fixes the functional regressions keeping performance improvement intact + adds a system test **Root Cause :** CSS performance improvement fix proposed in [JDK-8151756 ](https://bugs.openjdk.java.net/browse/JDK-8151756)correctly avoids the redundant CSS reapplication to children of a Parent. What was missed earlier in [JDK-8151756 ](https://bugs.openjdk.java.net/browse/JDK-8151756) fix : "CSS reapplication to the Parent itself”. This missing piece was the root cause of all functional regressions reported against [JDK-8151756](https://bugs.openjdk.java.net/browse/JDK-8151756) **Fix :** Fixed the identified root cause. See commit 2. **Testing :** 1. All passing unit tests continue to pass 2. New system test (based on [JDK-8209830](https://bugs.openjdk.java.net/browse/JDK-8209830)) added in this PR - fails before this fix and passes after the fix 3. System test JDK8183100Test continues to pass 4. All test cases attached to regressions [JDK-8185709](https://bugs.openjdk.java.net/browse/JDK-8185709), [JDK-8183100](https://bugs.openjdk.java.net/browse/JDK-8183100) and [JDK-8168951](https://bugs.openjdk.java.net/browse/JDK-8168951) pass with this fix In addition, testing by community with specific CSS performance / functionality will be helpful. Commits: - 12ea8220: Fix for functional regressions of JDK-8151756 + add a sytem test - d964675e: Reintroduce JDK-8151756 CSS performance fix Changes: https://git.openjdk.java.net/jfx/pull/34/files Webrev: https://webrevs.openjdk.java.net/jfx/34/webrev.00 Issue: https://bugs.openjdk.java.net/browse/JDK-8193445 Stats: 121 lines in 5 files changed: 104 ins; 0 del; 17 mod Patch: https://git.openjdk.java.net/jfx/pull/34.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/34/head:pull/34 >>> >>> While we are still discussing the fix itself, I added a few comments on >>> the new test. It generally looks good, but should be run on a variety >>> of systems, with and without the fix (once we have a final fix that we >>> are satisfied with). >>> >>> tests/system/src/test/java/test/robot/javafx/scene/CSSPerf_JDK8193445Test.java >>> line 26: >>> 25: 26: package test.robot.javafx.scene; 27: >>> >>> There is no need for this test to require robot. I recommend
Re: [Rev 03] RFR: 8200224: Multiple press event when JFXPanel gains focus
The pull request has been updated with a complete new set of changes (possibly due to a rebase). Commits: - 44774dfb: JDK-8200224 - d1309fb6: JDK-8200224 - 53a66b8f: JDK-8200224 - 0be3cb5b: JDK-8200224 - 03b59288: Merge branch 'master' of https://github.com/openjdk/jfx into JDK-8200224 - 5cd96a56: JDK-8200224 - 85c87628: JDK-8200224 - 314e423c: JDK-8200224 - 725e5fef: JDK-8200224 Changes: https://git.openjdk.java.net/jfx/pull/25/files Webrev: https://webrevs.openjdk.java.net/jfx/25/webrev.03 Issue: https://bugs.openjdk.java.net/browse/JDK-8200224 Stats: 272 lines in 3 files changed: 147 ins; 120 del; 5 mod Patch: https://git.openjdk.java.net/jfx/pull/25.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/25/head:pull/25 PR: https://git.openjdk.java.net/jfx/pull/25
Re: RFR: 8193445: JavaFX CSS is applied redundantly leading to significant performance degradation
On Tue, 26 Nov 2019 09:22:01 GMT, Ajit Ghaisas wrote:
> On Tue, 19 Nov 2019 14:29:16 GMT, David Grieve
> wrote:
>
>> On Tue, 19 Nov 2019 10:48:52 GMT, Ajit Ghaisas wrote:
>>
>>> On Fri, 15 Nov 2019 09:14:04 GMT, Ajit Ghaisas wrote:
>>>
On Thu, 14 Nov 2019 18:33:05 GMT, Kevin Rushforth wrote:
> On Tue, 12 Nov 2019 16:45:04 GMT, Ajit Ghaisas
> wrote:
>
>> **Issue :**
>> https://bugs.openjdk.java.net/browse/JDK-8193445
>>
>> **Background :**
>> The CSS performance improvement done in
>> [JDK-8151756](https://bugs.openjdk.java.net/browse/JDK-8151756) had to
>> be backed out due to functional regressions reported in
>> [JDK-8185709](https://bugs.openjdk.java.net/browse/JDK-8185709),
>> [JDK-8183100](https://bugs.openjdk.java.net/browse/JDK-8183100) and
>> [JDK-8168951](https://bugs.openjdk.java.net/browse/JDK-8168951).
>> Refer to [JDK-8183100](https://bugs.openjdk.java.net/browse/JDK-8183100)
>> for more details on this backout.
>>
>> **Description :**
>> This PR reintroduces the CSS performance improvement fix done in
>> [JDK-8151756](https://bugs.openjdk.java.net/browse/JDK-8151756) while
>> addressing the functional regressions that were reported in
>> [JDK-8185709](https://bugs.openjdk.java.net/browse/JDK-8185709),
>> [JDK-8183100](https://bugs.openjdk.java.net/browse/JDK-8183100) and
>> [JDK-8168951](https://bugs.openjdk.java.net/browse/JDK-8168951).
>> For ease of review, I have made two separate commits -
>> 1) [Commit
>> 1](https://github.com/openjdk/jfx/pull/34/commits/d964675ebc2a42f2fd6928b773819502683f2334)
>> - Reintroduces the CSS performance improvement fix done in
>> [JDK-8151756](https://bugs.openjdk.java.net/browse/JDK-8151756) - most
>> of the patch applied cleanly.
>> 2) [Commit 2
>> ](https://github.com/openjdk/jfx/pull/34/commits/12ea8220a730ff8d98d520ce870691d54f0de00f)-
>> fixes the functional regressions keeping performance improvement intact
>> + adds a system test
>>
>> **Root Cause :**
>> CSS performance improvement fix proposed in [JDK-8151756
>> ](https://bugs.openjdk.java.net/browse/JDK-8151756)correctly avoids the
>> redundant CSS reapplication to children of a Parent.
>> What was missed earlier in [JDK-8151756
>> ](https://bugs.openjdk.java.net/browse/JDK-8151756) fix : "CSS
>> reapplication to the Parent itself”.
>> This missing piece was the root cause of all functional regressions
>> reported against
>> [JDK-8151756](https://bugs.openjdk.java.net/browse/JDK-8151756)
>>
>> **Fix :**
>> Fixed the identified root cause. See commit 2.
>>
>> **Testing :**
>> 1. All passing unit tests continue to pass
>> 2. New system test (based on
>> [JDK-8209830](https://bugs.openjdk.java.net/browse/JDK-8209830)) added
>> in this PR - fails before this fix and passes after the fix
>> 3. System test JDK8183100Test continues to pass
>> 4. All test cases attached to regressions
>> [JDK-8185709](https://bugs.openjdk.java.net/browse/JDK-8185709),
>> [JDK-8183100](https://bugs.openjdk.java.net/browse/JDK-8183100) and
>> [JDK-8168951](https://bugs.openjdk.java.net/browse/JDK-8168951) pass
>> with this fix
>>
>> In addition, testing by community with specific CSS performance /
>> functionality will be helpful.
>>
>>
>>
>> Commits:
>> - 12ea8220: Fix for functional regressions of JDK-8151756 + add a sytem
>> test
>> - d964675e: Reintroduce JDK-8151756 CSS performance fix
>>
>> Changes: https://git.openjdk.java.net/jfx/pull/34/files
>> Webrev: https://webrevs.openjdk.java.net/jfx/34/webrev.00
>> Issue: https://bugs.openjdk.java.net/browse/JDK-8193445
>> Stats: 121 lines in 5 files changed: 104 ins; 0 del; 17 mod
>> Patch: https://git.openjdk.java.net/jfx/pull/34.diff
>> Fetch: git fetch https://git.openjdk.java.net/jfx pull/34/head:pull/34
>
> While we are still discussing the fix itself, I added a few comments on
> the new test. It generally looks good, but should be run on a variety of
> systems, with and without the fix (once we have a final fix that we are
> satisfied with).
>
> tests/system/src/test/java/test/robot/javafx/scene/CSSPerf_JDK8193445Test.java
> line 26:
>
>> 25:
>> 26: package test.robot.javafx.scene;
>> 27:
>
> There is no need for this test to require robot. I recommend moving it to
> `test.javafx.scene` (and not inherit from `VisualTestBase`).
>
> tests/system/src/test/java/test/robot/javafx/scene/CSSPerf_JDK8193445Test.java
> line 55:
>
>> 54:
>> 55: public class CSSPerf_JDK8193445Test extends VisualTestBase {
>> 56:
>
> We have moved away from putting the bug ID in the test
Re: RFR: 8193445: JavaFX CSS is applied redundantly leading to significant performance degradation
On Tue, 26 Nov 2019 09:22:01 GMT, David Grieve wrote: > On Tue, 26 Nov 2019 09:22:01 GMT, Ajit Ghaisas wrote: > >> On Tue, 19 Nov 2019 14:29:16 GMT, David Grieve >> wrote: >> >>> On Tue, 19 Nov 2019 10:48:52 GMT, Ajit Ghaisas wrote: >>> On Fri, 15 Nov 2019 09:14:04 GMT, Ajit Ghaisas wrote: > On Thu, 14 Nov 2019 18:33:05 GMT, Kevin Rushforth > wrote: > >> On Tue, 12 Nov 2019 16:45:04 GMT, Ajit Ghaisas >> wrote: >> >>> **Issue :** >>> https://bugs.openjdk.java.net/browse/JDK-8193445 >>> >>> **Background :** >>> The CSS performance improvement done in >>> [JDK-8151756](https://bugs.openjdk.java.net/browse/JDK-8151756) had to >>> be backed out due to functional regressions reported in >>> [JDK-8185709](https://bugs.openjdk.java.net/browse/JDK-8185709), >>> [JDK-8183100](https://bugs.openjdk.java.net/browse/JDK-8183100) and >>> [JDK-8168951](https://bugs.openjdk.java.net/browse/JDK-8168951). >>> Refer to >>> [JDK-8183100](https://bugs.openjdk.java.net/browse/JDK-8183100) for >>> more details on this backout. >>> >>> **Description :** >>> This PR reintroduces the CSS performance improvement fix done in >>> [JDK-8151756](https://bugs.openjdk.java.net/browse/JDK-8151756) while >>> addressing the functional regressions that were reported in >>> [JDK-8185709](https://bugs.openjdk.java.net/browse/JDK-8185709), >>> [JDK-8183100](https://bugs.openjdk.java.net/browse/JDK-8183100) and >>> [JDK-8168951](https://bugs.openjdk.java.net/browse/JDK-8168951). >>> For ease of review, I have made two separate commits - >>> 1) [Commit >>> 1](https://github.com/openjdk/jfx/pull/34/commits/d964675ebc2a42f2fd6928b773819502683f2334) >>> - Reintroduces the CSS performance improvement fix done in >>> [JDK-8151756](https://bugs.openjdk.java.net/browse/JDK-8151756) - most >>> of the patch applied cleanly. >>> 2) [Commit 2 >>> ](https://github.com/openjdk/jfx/pull/34/commits/12ea8220a730ff8d98d520ce870691d54f0de00f)- >>> fixes the functional regressions keeping performance improvement >>> intact + adds a system test >>> >>> **Root Cause :** >>> CSS performance improvement fix proposed in [JDK-8151756 >>> ](https://bugs.openjdk.java.net/browse/JDK-8151756)correctly avoids the >>> redundant CSS reapplication to children of a Parent. >>> What was missed earlier in [JDK-8151756 >>> ](https://bugs.openjdk.java.net/browse/JDK-8151756) fix : "CSS >>> reapplication to the Parent itself”. >>> This missing piece was the root cause of all functional regressions >>> reported against >>> [JDK-8151756](https://bugs.openjdk.java.net/browse/JDK-8151756) >>> >>> **Fix :** >>> Fixed the identified root cause. See commit 2. >>> >>> **Testing :** >>> 1. All passing unit tests continue to pass >>> 2. New system test (based on >>> [JDK-8209830](https://bugs.openjdk.java.net/browse/JDK-8209830)) added >>> in this PR - fails before this fix and passes after the fix >>> 3. System test JDK8183100Test continues to pass >>> 4. All test cases attached to regressions >>> [JDK-8185709](https://bugs.openjdk.java.net/browse/JDK-8185709), >>> [JDK-8183100](https://bugs.openjdk.java.net/browse/JDK-8183100) and >>> [JDK-8168951](https://bugs.openjdk.java.net/browse/JDK-8168951) pass >>> with this fix >>> >>> In addition, testing by community with specific CSS performance / >>> functionality will be helpful. >>> >>> >>> >>> Commits: >>> - 12ea8220: Fix for functional regressions of JDK-8151756 + add a >>> sytem test >>> - d964675e: Reintroduce JDK-8151756 CSS performance fix >>> >>> Changes: https://git.openjdk.java.net/jfx/pull/34/files >>> Webrev: https://webrevs.openjdk.java.net/jfx/34/webrev.00 >>> Issue: https://bugs.openjdk.java.net/browse/JDK-8193445 >>> Stats: 121 lines in 5 files changed: 104 ins; 0 del; 17 mod >>> Patch: https://git.openjdk.java.net/jfx/pull/34.diff >>> Fetch: git fetch https://git.openjdk.java.net/jfx pull/34/head:pull/34 >> >> While we are still discussing the fix itself, I added a few comments on >> the new test. It generally looks good, but should be run on a variety of >> systems, with and without the fix (once we have a final fix that we are >> satisfied with). >> >> tests/system/src/test/java/test/robot/javafx/scene/CSSPerf_JDK8193445Test.java >> line 26: >> >>> 25: >>> 26: package test.robot.javafx.scene; >>> 27: >> >> There is no need for this test to require robot. I recommend moving it >> to `test.javafx.scene` (and not inherit from `VisualTestBase`). >> >> tests/system/src/test/java/test/robot/javafx/scene/CSSPerf_JDK8193445Test.java >>
Re: RFR: 8193445: JavaFX CSS is applied redundantly leading to significant performance degradation
On Tue, 26 Nov 2019 09:22:02 GMT, Geoff wrote: > On Tue, 26 Nov 2019 09:22:02 GMT, Ajit Ghaisas wrote: > >> On Tue, 26 Nov 2019 09:22:01 GMT, David Grieve wrote: >> >>> On Tue, 26 Nov 2019 09:22:01 GMT, Ajit Ghaisas wrote: >>> On Tue, 19 Nov 2019 14:29:16 GMT, David Grieve wrote: > On Tue, 19 Nov 2019 10:48:52 GMT, Ajit Ghaisas > wrote: > >> On Fri, 15 Nov 2019 09:14:04 GMT, Ajit Ghaisas >> wrote: >> >>> On Thu, 14 Nov 2019 18:33:05 GMT, Kevin Rushforth >>> wrote: >>> On Tue, 12 Nov 2019 16:45:04 GMT, Ajit Ghaisas wrote: > **Issue :** > https://bugs.openjdk.java.net/browse/JDK-8193445 > > **Background :** > The CSS performance improvement done in > [JDK-8151756](https://bugs.openjdk.java.net/browse/JDK-8151756) had > to be backed out due to functional regressions reported in > [JDK-8185709](https://bugs.openjdk.java.net/browse/JDK-8185709), > [JDK-8183100](https://bugs.openjdk.java.net/browse/JDK-8183100) and > [JDK-8168951](https://bugs.openjdk.java.net/browse/JDK-8168951). > Refer to > [JDK-8183100](https://bugs.openjdk.java.net/browse/JDK-8183100) for > more details on this backout. > > **Description :** > This PR reintroduces the CSS performance improvement fix done in > [JDK-8151756](https://bugs.openjdk.java.net/browse/JDK-8151756) while > addressing the functional regressions that were reported in > [JDK-8185709](https://bugs.openjdk.java.net/browse/JDK-8185709), > [JDK-8183100](https://bugs.openjdk.java.net/browse/JDK-8183100) and > [JDK-8168951](https://bugs.openjdk.java.net/browse/JDK-8168951). > For ease of review, I have made two separate commits - > 1) [Commit > 1](https://github.com/openjdk/jfx/pull/34/commits/d964675ebc2a42f2fd6928b773819502683f2334) > - Reintroduces the CSS performance improvement fix done in > [JDK-8151756](https://bugs.openjdk.java.net/browse/JDK-8151756) - > most of the patch applied cleanly. > 2) [Commit 2 > ](https://github.com/openjdk/jfx/pull/34/commits/12ea8220a730ff8d98d520ce870691d54f0de00f)- > fixes the functional regressions keeping performance improvement > intact + adds a system test > > **Root Cause :** > CSS performance improvement fix proposed in [JDK-8151756 > ](https://bugs.openjdk.java.net/browse/JDK-8151756)correctly avoids > the redundant CSS reapplication to children of a Parent. > What was missed earlier in [JDK-8151756 > ](https://bugs.openjdk.java.net/browse/JDK-8151756) fix : "CSS > reapplication to the Parent itself”. > This missing piece was the root cause of all functional regressions > reported against > [JDK-8151756](https://bugs.openjdk.java.net/browse/JDK-8151756) > > **Fix :** > Fixed the identified root cause. See commit 2. > > **Testing :** > 1. All passing unit tests continue to pass > 2. New system test (based on > [JDK-8209830](https://bugs.openjdk.java.net/browse/JDK-8209830)) > added in this PR - fails before this fix and passes after the fix > 3. System test JDK8183100Test continues to pass > 4. All test cases attached to regressions > [JDK-8185709](https://bugs.openjdk.java.net/browse/JDK-8185709), > [JDK-8183100](https://bugs.openjdk.java.net/browse/JDK-8183100) and > [JDK-8168951](https://bugs.openjdk.java.net/browse/JDK-8168951) pass > with this fix > > In addition, testing by community with specific CSS performance / > functionality will be helpful. > > > > Commits: > - 12ea8220: Fix for functional regressions of JDK-8151756 + add a > sytem test > - d964675e: Reintroduce JDK-8151756 CSS performance fix > > Changes: https://git.openjdk.java.net/jfx/pull/34/files > Webrev: https://webrevs.openjdk.java.net/jfx/34/webrev.00 > Issue: https://bugs.openjdk.java.net/browse/JDK-8193445 > Stats: 121 lines in 5 files changed: 104 ins; 0 del; 17 mod > Patch: https://git.openjdk.java.net/jfx/pull/34.diff > Fetch: git fetch https://git.openjdk.java.net/jfx > pull/34/head:pull/34 While we are still discussing the fix itself, I added a few comments on the new test. It generally looks good, but should be run on a variety of systems, with and without the fix (once we have a final fix that we are satisfied with). tests/system/src/test/java/test/robot/javafx/scene/CSSPerf_JDK8193445Test.java line 26:
Re: RFR: 8193445: JavaFX CSS is applied redundantly leading to significant performance degradation
On Tue, 12 Nov 2019 16:45:04 GMT, Ajit Ghaisas wrote:
> **Issue :**
> https://bugs.openjdk.java.net/browse/JDK-8193445
>
> **Background :**
> The CSS performance improvement done in
> [JDK-8151756](https://bugs.openjdk.java.net/browse/JDK-8151756) had to be
> backed out due to functional regressions reported in
> [JDK-8185709](https://bugs.openjdk.java.net/browse/JDK-8185709),
> [JDK-8183100](https://bugs.openjdk.java.net/browse/JDK-8183100) and
> [JDK-8168951](https://bugs.openjdk.java.net/browse/JDK-8168951).
> Refer to [JDK-8183100](https://bugs.openjdk.java.net/browse/JDK-8183100) for
> more details on this backout.
>
> **Description :**
> This PR reintroduces the CSS performance improvement fix done in
> [JDK-8151756](https://bugs.openjdk.java.net/browse/JDK-8151756) while
> addressing the functional regressions that were reported in
> [JDK-8185709](https://bugs.openjdk.java.net/browse/JDK-8185709),
> [JDK-8183100](https://bugs.openjdk.java.net/browse/JDK-8183100) and
> [JDK-8168951](https://bugs.openjdk.java.net/browse/JDK-8168951).
> For ease of review, I have made two separate commits -
> 1) [Commit
> 1](https://github.com/openjdk/jfx/pull/34/commits/d964675ebc2a42f2fd6928b773819502683f2334)
> - Reintroduces the CSS performance improvement fix done in
> [JDK-8151756](https://bugs.openjdk.java.net/browse/JDK-8151756) - most of the
> patch applied cleanly.
> 2) [Commit 2
> ](https://github.com/openjdk/jfx/pull/34/commits/12ea8220a730ff8d98d520ce870691d54f0de00f)-
> fixes the functional regressions keeping performance improvement intact +
> adds a system test
>
> **Root Cause :**
> CSS performance improvement fix proposed in [JDK-8151756
> ](https://bugs.openjdk.java.net/browse/JDK-8151756)correctly avoids the
> redundant CSS reapplication to children of a Parent.
> What was missed earlier in [JDK-8151756
> ](https://bugs.openjdk.java.net/browse/JDK-8151756) fix : "CSS reapplication
> to the Parent itself”.
> This missing piece was the root cause of all functional regressions reported
> against [JDK-8151756](https://bugs.openjdk.java.net/browse/JDK-8151756)
>
> **Fix :**
> Fixed the identified root cause. See commit 2.
>
> **Testing :**
> 1. All passing unit tests continue to pass
> 2. New system test (based on
> [JDK-8209830](https://bugs.openjdk.java.net/browse/JDK-8209830)) added in
> this PR - fails before this fix and passes after the fix
> 3. System test JDK8183100Test continues to pass
> 4. All test cases attached to regressions
> [JDK-8185709](https://bugs.openjdk.java.net/browse/JDK-8185709),
> [JDK-8183100](https://bugs.openjdk.java.net/browse/JDK-8183100) and
> [JDK-8168951](https://bugs.openjdk.java.net/browse/JDK-8168951) pass with
> this fix
>
> In addition, testing by community with specific CSS performance /
> functionality will be helpful.
>
>
>
> Commits:
> - 12ea8220: Fix for functional regressions of JDK-8151756 + add a sytem test
> - d964675e: Reintroduce JDK-8151756 CSS performance fix
>
> Changes: https://git.openjdk.java.net/jfx/pull/34/files
> Webrev: https://webrevs.openjdk.java.net/jfx/34/webrev.00
> Issue: https://bugs.openjdk.java.net/browse/JDK-8193445
> Stats: 121 lines in 5 files changed: 104 ins; 0 del; 17 mod
> Patch: https://git.openjdk.java.net/jfx/pull/34.diff
> Fetch: git fetch https://git.openjdk.java.net/jfx pull/34/head:pull/34
The fix itself looks good and is a much safer approach than the previous one.
I've done a fair bit of testing and can see no regressions as a result of this
fix. I did find one unrelated issue while testing (a visual bug introduced back
in JDK 10) that I will file separately.
The test is pretty close, but still needs a few changes. The main problem is
that it does not catch the performance problem, meaning if you run it without
the fix, it will still pass. Your test class extends
`javafx.application.Application`, which can cause problems, since JUnit will
create a new instance of the test class that is different from the one created
by the call to `Application.launch` in the `@BeforeClass` method. This in turn
leads to the `rootPane` instance used by the test method being different from
the one used as the root of the visible scene. The fix is to use a separate
nested (static) subclass of Application, and make the rootPane field a static
field. I have left inline comments for this and a few other things I noticed.
tests/system/src/test/java/test/javafx/scene/QuadraticCssTimeTest.java line 63:
> 62:
> 63: public class QuadraticCssTimeTest extends Application {
> 64:
Remove `extends Application`
tests/system/src/test/java/test/javafx/scene/QuadraticCssTimeTest.java line 66:
> 65: static private CountDownLatch startupLatch;
> 66: static private Stage stage;
> 67: private BorderPane rootPane = new BorderPane();
Minor: our convention is `private static`.
tests/system/src/test/java/test/javafx/scene/QuadraticCssTimeTest.java line 67:
>
Re: [Approved] RFR: 8193445: JavaFX CSS is applied redundantly leading to significant performance degradation
On Tue, 12 Nov 2019 16:45:04 GMT, Ajit Ghaisas wrote: > **Issue :** > https://bugs.openjdk.java.net/browse/JDK-8193445 > > **Background :** > The CSS performance improvement done in > [JDK-8151756](https://bugs.openjdk.java.net/browse/JDK-8151756) had to be > backed out due to functional regressions reported in > [JDK-8185709](https://bugs.openjdk.java.net/browse/JDK-8185709), > [JDK-8183100](https://bugs.openjdk.java.net/browse/JDK-8183100) and > [JDK-8168951](https://bugs.openjdk.java.net/browse/JDK-8168951). > Refer to [JDK-8183100](https://bugs.openjdk.java.net/browse/JDK-8183100) for > more details on this backout. > > **Description :** > This PR reintroduces the CSS performance improvement fix done in > [JDK-8151756](https://bugs.openjdk.java.net/browse/JDK-8151756) while > addressing the functional regressions that were reported in > [JDK-8185709](https://bugs.openjdk.java.net/browse/JDK-8185709), > [JDK-8183100](https://bugs.openjdk.java.net/browse/JDK-8183100) and > [JDK-8168951](https://bugs.openjdk.java.net/browse/JDK-8168951). > For ease of review, I have made two separate commits - > 1) [Commit > 1](https://github.com/openjdk/jfx/pull/34/commits/d964675ebc2a42f2fd6928b773819502683f2334) > - Reintroduces the CSS performance improvement fix done in > [JDK-8151756](https://bugs.openjdk.java.net/browse/JDK-8151756) - most of the > patch applied cleanly. > 2) [Commit 2 > ](https://github.com/openjdk/jfx/pull/34/commits/12ea8220a730ff8d98d520ce870691d54f0de00f)- > fixes the functional regressions keeping performance improvement intact + > adds a system test > > **Root Cause :** > CSS performance improvement fix proposed in [JDK-8151756 > ](https://bugs.openjdk.java.net/browse/JDK-8151756)correctly avoids the > redundant CSS reapplication to children of a Parent. > What was missed earlier in [JDK-8151756 > ](https://bugs.openjdk.java.net/browse/JDK-8151756) fix : "CSS reapplication > to the Parent itself”. > This missing piece was the root cause of all functional regressions reported > against [JDK-8151756](https://bugs.openjdk.java.net/browse/JDK-8151756) > > **Fix :** > Fixed the identified root cause. See commit 2. > > **Testing :** > 1. All passing unit tests continue to pass > 2. New system test (based on > [JDK-8209830](https://bugs.openjdk.java.net/browse/JDK-8209830)) added in > this PR - fails before this fix and passes after the fix > 3. System test JDK8183100Test continues to pass > 4. All test cases attached to regressions > [JDK-8185709](https://bugs.openjdk.java.net/browse/JDK-8185709), > [JDK-8183100](https://bugs.openjdk.java.net/browse/JDK-8183100) and > [JDK-8168951](https://bugs.openjdk.java.net/browse/JDK-8168951) pass with > this fix > > In addition, testing by community with specific CSS performance / > functionality will be helpful. > > > > Commits: > - 12ea8220: Fix for functional regressions of JDK-8151756 + add a sytem test > - d964675e: Reintroduce JDK-8151756 CSS performance fix > > Changes: https://git.openjdk.java.net/jfx/pull/34/files > Webrev: https://webrevs.openjdk.java.net/jfx/34/webrev.00 > Issue: https://bugs.openjdk.java.net/browse/JDK-8193445 > Stats: 121 lines in 5 files changed: 104 ins; 0 del; 17 mod > Patch: https://git.openjdk.java.net/jfx/pull/34.diff > Fetch: git fetch https://git.openjdk.java.net/jfx pull/34/head:pull/34 Approved by dgrieve (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/34
Re: [Rev 01] RFR: 8193445: JavaFX CSS is applied redundantly leading to significant performance degradation
The pull request has been updated with additional changes. Added commits: - 2054da4c: Address review comments on test - 4dade6e5: Simpler fix + System test corrections - bd4a306a: Revert commit1 and commit 2 Changes: - all: https://git.openjdk.java.net/jfx/pull/34/files - new: https://git.openjdk.java.net/jfx/pull/34/files/12ea8220..2054da4c Webrevs: - full: https://webrevs.openjdk.java.net/jfx/34/webrev.01 - incr: https://webrevs.openjdk.java.net/jfx/34/webrev.00-01 Issue: https://bugs.openjdk.java.net/browse/JDK-8193445 Stats: 253 lines in 6 files changed: 132 ins; 104 del; 17 mod Patch: https://git.openjdk.java.net/jfx/pull/34.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/34/head:pull/34 PR: https://git.openjdk.java.net/jfx/pull/34
Re: RFR: 8193445: JavaFX CSS is applied redundantly leading to significant performance degradation
On Tue, 19 Nov 2019 14:29:16 GMT, David Grieve
wrote:
> On Tue, 19 Nov 2019 10:48:52 GMT, Ajit Ghaisas wrote:
>
>> On Fri, 15 Nov 2019 09:14:04 GMT, Ajit Ghaisas wrote:
>>
>>> On Thu, 14 Nov 2019 18:33:05 GMT, Kevin Rushforth wrote:
>>>
On Tue, 12 Nov 2019 16:45:04 GMT, Ajit Ghaisas
wrote:
> **Issue :**
> https://bugs.openjdk.java.net/browse/JDK-8193445
>
> **Background :**
> The CSS performance improvement done in
> [JDK-8151756](https://bugs.openjdk.java.net/browse/JDK-8151756) had to be
> backed out due to functional regressions reported in
> [JDK-8185709](https://bugs.openjdk.java.net/browse/JDK-8185709),
> [JDK-8183100](https://bugs.openjdk.java.net/browse/JDK-8183100) and
> [JDK-8168951](https://bugs.openjdk.java.net/browse/JDK-8168951).
> Refer to [JDK-8183100](https://bugs.openjdk.java.net/browse/JDK-8183100)
> for more details on this backout.
>
> **Description :**
> This PR reintroduces the CSS performance improvement fix done in
> [JDK-8151756](https://bugs.openjdk.java.net/browse/JDK-8151756) while
> addressing the functional regressions that were reported in
> [JDK-8185709](https://bugs.openjdk.java.net/browse/JDK-8185709),
> [JDK-8183100](https://bugs.openjdk.java.net/browse/JDK-8183100) and
> [JDK-8168951](https://bugs.openjdk.java.net/browse/JDK-8168951).
> For ease of review, I have made two separate commits -
> 1) [Commit
> 1](https://github.com/openjdk/jfx/pull/34/commits/d964675ebc2a42f2fd6928b773819502683f2334)
> - Reintroduces the CSS performance improvement fix done in
> [JDK-8151756](https://bugs.openjdk.java.net/browse/JDK-8151756) - most of
> the patch applied cleanly.
> 2) [Commit 2
> ](https://github.com/openjdk/jfx/pull/34/commits/12ea8220a730ff8d98d520ce870691d54f0de00f)-
> fixes the functional regressions keeping performance improvement intact
> + adds a system test
>
> **Root Cause :**
> CSS performance improvement fix proposed in [JDK-8151756
> ](https://bugs.openjdk.java.net/browse/JDK-8151756)correctly avoids the
> redundant CSS reapplication to children of a Parent.
> What was missed earlier in [JDK-8151756
> ](https://bugs.openjdk.java.net/browse/JDK-8151756) fix : "CSS
> reapplication to the Parent itself”.
> This missing piece was the root cause of all functional regressions
> reported against
> [JDK-8151756](https://bugs.openjdk.java.net/browse/JDK-8151756)
>
> **Fix :**
> Fixed the identified root cause. See commit 2.
>
> **Testing :**
> 1. All passing unit tests continue to pass
> 2. New system test (based on
> [JDK-8209830](https://bugs.openjdk.java.net/browse/JDK-8209830)) added in
> this PR - fails before this fix and passes after the fix
> 3. System test JDK8183100Test continues to pass
> 4. All test cases attached to regressions
> [JDK-8185709](https://bugs.openjdk.java.net/browse/JDK-8185709),
> [JDK-8183100](https://bugs.openjdk.java.net/browse/JDK-8183100) and
> [JDK-8168951](https://bugs.openjdk.java.net/browse/JDK-8168951) pass with
> this fix
>
> In addition, testing by community with specific CSS performance /
> functionality will be helpful.
>
>
>
> Commits:
> - 12ea8220: Fix for functional regressions of JDK-8151756 + add a sytem
> test
> - d964675e: Reintroduce JDK-8151756 CSS performance fix
>
> Changes: https://git.openjdk.java.net/jfx/pull/34/files
> Webrev: https://webrevs.openjdk.java.net/jfx/34/webrev.00
> Issue: https://bugs.openjdk.java.net/browse/JDK-8193445
> Stats: 121 lines in 5 files changed: 104 ins; 0 del; 17 mod
> Patch: https://git.openjdk.java.net/jfx/pull/34.diff
> Fetch: git fetch https://git.openjdk.java.net/jfx pull/34/head:pull/34
While we are still discussing the fix itself, I added a few comments on
the new test. It generally looks good, but should be run on a variety of
systems, with and without the fix (once we have a final fix that we are
satisfied with).
tests/system/src/test/java/test/robot/javafx/scene/CSSPerf_JDK8193445Test.java
line 26:
> 25:
> 26: package test.robot.javafx.scene;
> 27:
There is no need for this test to require robot. I recommend moving it to
`test.javafx.scene` (and not inherit from `VisualTestBase`).
tests/system/src/test/java/test/robot/javafx/scene/CSSPerf_JDK8193445Test.java
line 55:
> 54:
> 55: public class CSSPerf_JDK8193445Test extends VisualTestBase {
> 56:
We have moved away from putting the bug ID in the test class name, so I
recommend renaming it.
tests/system/src/test/java/test/robot/javafx/scene/CSSPerf_JDK8193445Test.java
line 78:
> 77:
