• Type: Enhancement
• Component: javafx
• Sub-Component: graphics

• Priority: P3
• Status: Resolved
• Resolution: Fixed

• Submitted: 2016-04-25
• Updated: 2016-04-27
• Resolved: 2016-04-27

JDK 9
9Fixed

Changeset: 2ba17bc472f7 Author: ckyang Date: 2016-04-27 16:35 -0700 URL: http://hg.openjdk.java.net/openjfx/9-dev/rt/rev/2ba17bc472f7 27-04-2016 +1 27-04-2016 This new webrev took all feedback except 1) The removal of EffectShim.java which I prefer to handle it with a separate JIRA. 2) On the last feedback, regarding the breaking of line on '=', we have look at our code base and found that both before and after are acceptable style. Netbeans will reformat to before so it will be odd to fight against the IDE we use. http://cr.openjdk.java.net/~ckyang/JDK-8155053/webrev.01/ 27-04-2016 I ran a full set of tests in jigsaw mode and everything runs fine. Here are my review comments: Bugs ---- modules/graphics/src/main/java/com/sun/scenario/effect/EffectHelper.java The following initializes the wrong class: 41 static { 42 forceInit(Effect.class); 43 } 44 This should be javafx.scene.effect.Effect.class Other issues (mostly minor) --------------------------- modules/graphics/src/main/java/javafx/scene/effect/Effect.java * The package name is not needed when referring to javafx.scene.effect.Effect; it can be referred to simply as "Effect" since we are in that class/package (you only need to qualify com.sun.scenario.effect.Effect in the return type of the getPeer method). + EffectHelper.setEffectAccessor(new EffectHelper.EffectAccessor() { + public com.sun.scenario.effect.Effect getPeer(javafx.scene.effect.Effect effect) { + public void sync(javafx.scene.effect.Effect effect) { + public IntegerProperty effectDirtyProperty(javafx.scene.effect.Effect effect) { + public boolean isEffectDirty(javafx.scene.effect.Effect effect) { + public BaseBounds getBounds(javafx.scene.effect.Effect effect, BaseBounds bounds, + public javafx.scene.effect.Effect copy(javafx.scene.effect.Effect effect) { * Indentation is wrong for the following: One too many leading spaces: private com.sun.scenario.effect.Effect peer; abstract com.sun.scenario.effect.Effect createPeer(); com.sun.scenario.effect.Effect getPeer() { It would look better to line up the arguments so that BaseTransform is directly below BaseBounds (this applies to the subclasses, too) + abstract BaseBounds getBounds(BaseBounds bounds, BaseTransform tx, Node node, BoundsAccessor boundsAccessor); modules/graphics/src/test/java/javafx/scene/effect/EffectShim.java * You might be able to simply remove this class and all subclasses, since its only method -- getPeer(Effect) -- is now covered by EffectHelper.getPeer(Effect). It would be OK to leave that for another pass if you prefer. modules/web/src/ios/java/com/sun/javafx/scene/web/behavior/HTMLEditorBehavior.java * Bad indentation: + new KeyMapping(new KeyBinding(F12), e -> ParentHelper.getTraversalEngine(getNode()).selectFirst().requestFocus()), new KeyMapping(new KeyBinding(TAB).ctrl(), FocusTraversalInputMap::traverseNext), modules/controls/src/main/java/javafx/scene/control/skin/ComboBoxPopupControl.java modules/controls/src/main/java/javafx/scene/control/skin/SpinnerSkin.java * A blank line before the @Override line might aid readability give that you split the line: + ParentHelper.setTraversalEngine(comboBoxBase, + new ParentTraversalEngine(comboBoxBase, new Algorithm() { @Override public Node select(Node owner, Direction dir, TraversalContext context) { modules/graphics/src/main/java/com/sun/javafx/scene/traversal/TabOrderHelper.java * We would generally break a line like this either after the '=' or before the '?' : + final ParentTraversalEngine parentEngine + = n instanceof Parent ? ParentHelper.getTraversalEngine((Parent)n) : null; 27-04-2016 Besides removing the word impl_ from methods and I also renamed XXXImpl() to XXXPeer() to better reflect the meaning of these methods. webrev: http://cr.openjdk.java.net/~ckyang/JDK-8155053/webrev.00/ 26-04-2016