JDK-8158659 : Encapsulate impl_ methods in Node and its subclasses
  • Type: Enhancement
  • Component: javafx
  • Sub-Component: graphics
  • Priority: P3
  • Status: Resolved
  • Resolution: Fixed
  • Submitted: 2016-06-03
  • Updated: 2016-06-20
  • Resolved: 2016-06-07
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
JDK 9
9Fixed
Related Reports
Blocks :  
JDK-8144585 - Encapsulate JavaFX impl_* implementation methods
Relates :  
JDK-8089509 - Javadoc of custom controls shows links to impl_* methods of its JavaFX superclasses
Relates :  
JDK-8159383 - Mouse hover on Webview scroll bar leads to NPE in ScrollBarThemeImpl.hitTest
Description
This is to make JDK-8144585 more manageable and easier for reviewer by breaking it into smaller pieces of work. This work is a follow on of JDK-8157900 to make Node and its subclasses treatasprivate clean.
Comments
Thanks! I have added the missing comment to Parent and PopupControl in my committed change. I also filed a P2 bug to address the issue created by this fix: JDK-8158948
07-06-2016
Changeset: b20722d86cf8 Author: ckyang Date: 2016-06-07 09:51 -0700 URL: http://hg.openjdk.java.net/openjfx/9-dev/rt/rev/b20722d86cf8
07-06-2016
+1
07-06-2016
Looks good. I see one remaining issue (minor). The doGetAllParentStylesheets method in Parent.java should have the comment: "This method MUST only be called via its accessor...". Ditto for PopupControl.java. No need for a new webrev to add the two missing comments, assuming no other issues are raised. +1
07-06-2016
Thanks for the valuable feedback. Here is webrev.02: http://cr.openjdk.java.net/~ckyang/JDK-8158659/webrev.02/ Here is the delta between 01 and 02: http://cr.openjdk.java.net/~ckyang/JDK-8158659/webrev-01_02.diff/ @Jonathan: SB is pretty much unusable without access to those impl_ methods ( and the printlns). I will file a P2 bug to fix SB ASAP once this is approved. Yes, I did run full test with this webrev and VirtualFlowTest works as before. That line I removed was a commented line.
07-06-2016
In addition to what Vadim pointed out, the following in Parent.java should be renamed "doGetAllParentStylesheets" with the note that it must only be called via its accessor method: + private List<String> getAllParentStylesheets() {
06-06-2016
Comments: 1) In Scene Builder MetaData class you're hiding the properties (because that is what was done when they were impl properties), but now that they are public API, they probably should not be in the hiddenProperties list anymore. 2) In Scene Builder Deprecation class, have you investigated whether the loss of that functionality (and the printlns) will have any impact on the usage of the app? 3) FocusTraversalInputMap has modified the javadoc comment to refer to NodeHelper - this shouldn't be mentioned as it isn't public API. 4) VirtualFlowTest - I presume you've run all unit tests to make sure they still pass without the removed line?
06-06-2016
NodeHelper.getMatchingStyles seems to be calling itself rather than nodeAccessor. style issues; PathHelper:71 - excessive space after opening bracket. Node.java at lines 437, 443, 530 and 587 could use more tabs. Some more files have this issue at methods doComputeGeomBounds and doComputeIntersects of accessors. Region.java at 3170 - private final. More private finals in Text.java.
06-06-2016
1) Fixed. 2) Fixed. 3) Fixed. I've verified that that was the only one. 4) Yes, I'm aware. I prefer to keeping the pattern for better code maintainability in the future. 5) Thanks. This the one I would like to fix using the Helper.superXXXX pattern we did in Text but I miss it. Fixed. Here is webrev 01: http://cr.openjdk.java.net/~ckyang/JDK-8158659/webrev.01/ Here is the delta between 00 and 01: http://cr.openjdk.java.net/~ckyang/JDK-8158659/webrev-00_01.diff/ I have verified that this webrev passed on Linux (JIGSAW) and Mac (Non-JIGSAW) by running full test and run-subset scripts.
06-06-2016
PopupControl.java ----------------------- 1. BUG: the following function, defined in Parent, was previously overridden in PopupControl$CSSBridge: impl_getAllParentStylesheets As such this needs to use the *Impl (Helper) / do* pattern. So Parent and CSSBridge (and any other subclass that currently overrides impl_getAllParentStylesheets) need to turn it into a "private doGetAllParentStylesheets" method. ParentHelper will need a *Impl instance method that can be overridden by CSSBridgeHelper to call the right doGetAllParentStylesheets method. 2. Minor: you added an unused import: > import com.sun.javafx.util.Utils; Node.java ----------- 3. Minor: Some of the calls (the ones that are not do* methods) don't need to go through the helper. For example, the "isTreeVisible" method is called sometimes directly and sometimes via NodeHelper. + isTreeVisible = parentNode == null || NodeHelper.isTreeVisible(parentNode); ... + if (parentChanged && parentNode != null && parentNode.isTreeVisible() I don't know if there are others. Pane.java --------- 4. Question: why does it need to create an instance of PaneHelper if it doesn't do anything? You might be able to elimiate it (but then you would have the odd case of CSSBridgeHelper extending RegionHelper rather than PaneHelper). Might just want to leave it as is. Region.java ----------- 5. Comment: + private double bx1, by1, bx2, by2; Hmmm. This seems a bit fragile, especially with such generic names, although as long as you have verified that they do not conflict it should be OK. I recommend that you add a comment that these variables are only used during the two doComputeGeomBounds methods. Have you checked to ensure that computeBounds is not called reentrantly on the same Region instance? I doubt it would be, but best to be sure. Alternatively, you could rework this with a superComputeGeomBounds method, but that would be more work and might not be necessary. Follow-on bugs needed: 1. SceneBuilder CSS processing 2. GroupTest: layout bounds tests
03-06-2016
http://cr.openjdk.java.net/~ckyang/JDK-8158659/webrev.00/ graphics module will be @treatAsPrivate clean with this proposed fix. I follow up with another smaller proposed fix to clean up control, fxml, media and web modules. I have verified that this webrev passed on Linux (JIGSAW) and Mac (Non-JIGSAW) by running full test and run-subset scripts.
03-06-2016