On Wed, 13 Nov 2019 19:10:45 GMT, David Grieve <github.com+4412658+dsgri...@openjdk.org> wrote:
> On Tue, 12 Nov 2019 16:55:45 GMT, Ajit Ghaisas <aghai...@openjdk.org> wrote: > >> On Tue, 12 Nov 2019 16:52:54 GMT, Ajit Ghaisas <aghai...@openjdk.org> wrote: >> >>> On Tue, 12 Nov 2019 16:45:04 GMT, Ajit Ghaisas <aghai...@openjdk.org> 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 >>> >>> Reviewers: @kevinrushforth and @arapte >> >> It will be helpful if I can get some feedback from community with additional >> testing apart from what is mentioned in the description. > > Has this been checked with SubScene? Two cases would be 1) SubScene > inheriting a style from its parent, and 2) behavior of a parent in the > SubScene itself. I don't expect that this would be an issue, but there is > some special handling of CSS in SubScene IIRC. Having looked at this issue previously (https://mail.openjdk.java.net/pipermail/openjfx-dev/2018-November/022842.html and https://github.com/DeanWookey/openjdk-jfx/commit/65a1ed82bce262294f1969e9a12e1126ec8a1ec6), I'm a bit confused. Doesn't commit https://github.com/openjdk/jfx/pull/34/commits/12ea8220a730ff8d98d520ce870691d54f0de00f essentially add another reapplyCSS() above the scenesChanged call on line 1074? I'm guessing this works because reapplyCss() (different from reapplyCSS()) sets cssFlag to UPDATE, which then means subsequent calls to reapplyCSS() don't call reapplyCss()? Does this leave the whole tree with the CssFlags.REAPPLY set instead of CssFlags.UPDATE as they would be without these changes? I'm not sure about the impact of that. I'm also curious whether commit 1 is even necessary with commit 2. PR: https://git.openjdk.java.net/jfx/pull/34
