Re: Code Review Request For 8155757: Encapsulate impl_ methods in animation, canvas, image, input, layout, paint, and text packages

2016-04-29 Thread Jim Graham
One comment on the implementation.  For the methods used by an accessor inner class, if you make them private in the 
outer class then that inner class will need a hidden accessor method to be added in the bytecodes.  If you make them 
package-private then they can access the method directly.


Basically, an inner class is really "just another class in the package, but with a special name" and actually have no 
access to private methods in their outer classes at all, but javac works around this by adding a hidden method that has 
more open access and using that.


An example is Image.getPlatformImage() - you turned it from "public and impl_" into private.  Why not just leave it 
package-private/default access?


For example, compiling this class:

public class InnerPrivateTest {
private void foo() {}
public class InnerClass {
public void bar() { foo(); }
}
}

yields this byte code for InnerPrivateTest.class:

public class InnerPrivateTest {
  public InnerPrivateTest();
Code:
   0: aload_0
   1: invokespecial #2  // Method 
java/lang/Object."":()V
   4: return

  private void foo();
Code:
   0: return

  static void access$000(InnerPrivateTest);
Code:
   0: aload_0
   1: invokespecial #1  // Method foo:()V
   4: return
}

and this for the InnerClass:

public class InnerPrivateTest$InnerClass {
  final InnerPrivateTest this$0;

  public InnerPrivateTest$InnerClass(InnerPrivateTest);
Code:
   0: aload_0
   1: aload_1
   2: putfield  #1  // Field this$0:LInnerPrivateTest;
   5: aload_0
   6: invokespecial #2  // Method 
java/lang/Object."":()V
   9: return

  public void bar();
Code:
   0: aload_0
   1: getfield  #1  // Field this$0:LInnerPrivateTest;
   4: invokestatic  #3  // Method 
InnerPrivateTest.access$000:(LInnerPrivateTest;)V
   7: return
}

Changing the access on foo() to default (package private), yields this byte 
code:

public class InnerPackageTest {
  public InnerPackageTest();
Code:
   0: aload_0
   1: invokespecial #1  // Method 
java/lang/Object."":()V
   4: return

  void foo();
Code:
   0: return
}

public class InnerPackageTest$InnerClass {
  final InnerPackageTest this$0;

  public InnerPackageTest$InnerClass(InnerPackageTest);
Code:
   0: aload_0
   1: aload_1
   2: putfield  #1  // Field this$0:LInnerPackageTest;
   5: aload_0
   6: invokespecial #2  // Method 
java/lang/Object."":()V
   9: return

  public void bar();
Code:
   0: aload_0
   1: getfield  #1  // Field this$0:LInnerPackageTest;
   4: invokevirtual #3  // Method InnerPackageTest.foo:()V
   7: return
}

...jim

On 4/29/16 11:50 AM, Chien Yang wrote:

Hi Kevin,

Please review the proposed fix:

JIRA: https://bugs.openjdk.java.net/browse/JDK-8155757
Webrev: http://cr.openjdk.java.net/~ckyang/JDK-8155757/webrev.00/

Thanks,
- Chien


Code Review Request For 8155757: Encapsulate impl_ methods in animation, canvas, image, input, layout, paint, and text packages

2016-04-29 Thread Chien Yang

Hi Kevin,

Please review the proposed fix:

JIRA: https://bugs.openjdk.java.net/browse/JDK-8155757
Webrev: http://cr.openjdk.java.net/~ckyang/JDK-8155757/webrev.00/

Thanks,
- Chien


In(Sanity) Testing Mondays

2016-04-29 Thread Vadim Pakhnushev

Reminder, Monday is our weekly sanity testing.

You can find your testing assignment at:
https://wiki.openjdk.java.net/display/OpenJFX/Sanity+Testing

Also please remember that the repo will be locked from 1am PST until 1pm 
PST.


Happy testing!

Thanks,
Vadim