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

[Chien Yang]

Hi Jim,

Thanks for sharing this information and your thought. I'm not sure is 
this saving worth violating the principle of minimizing scope in code. I 
guess you did bring up a good point let me think over it and discuss 
with Kevin tomorrow.

- Chien

On 4/29/16, 4:04 PM, Jim Graham wrote:
> 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."<init>":()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."<init>":()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."<init>":()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."<init>":()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