[9] Code Review Request For 8155762: Encapsulate JavaFX impl_* implementation methods in transform package

[Chien Yang]

Hi Jim,

Just quick reply to some of your questions.

1) TransformUtils.java: My initial take was to delete this class but 
after chatting with Kevin, we decided to keep it in the interest of 
limiting the scope of this change. Moving those methods will touch much 
more files. We have about 2 weeks to finish the whole encapsulation of 
all JavaFX impl_* methods.

2) TransformHelper.java: This is the class naming pattern we use to 
encapsulate all accessor interface classes. Also only less than half of 
TransformUtils.java was moved into TransformHelper.java. A big portion, 
ImmutableTransform, has to move into Transform.java.

3) ImmutableTransform: Yes, this is unclean in my opinion when I was 
reading it. However this is design/implementation cleanup work is out of 
the scope of this JIRA. I would suggest we file a separate JIRA for it 
and since you are the code owner of Transform you can have it too! :-)

Lastly, JavaFX code review is done in the JIRA so that discussion of 
that issue will stay in that JIRA.

Thanks,
- Chien


On 5/6/16, 8:10 PM, Jim Graham wrote:
> TransformUtils.java - can't this class be deleted now since all it 
> exists to do is to forward to TransformHelper?
>
> Alternatively, why create TransformHelper in the first place when it 
> is 90% just a copy of what TransformUtils used to be (ignoring the 
> inner class that got moved to Transform) with an accessor interface 
> thrown in for good measure?  Just to change the name?  That could be 
> done by using a source rename rather than gutting the old file and 
> creating the new one from scratch...?  And we don't need both of them, 
> do we?
>
> Transform.java, line 2172 & 2187 - is there a reason to forward to the 
> factory rather than just instantiating directly?
>
> Transform.java, line 2177 & 2189 - I suppose the reason that the 
> arguments are not declared to be Immutable in the first place is that 
> some of these are used from other packages that have no visibility to 
> the Immutable.  I was going to say that we should protect against them 
> not being Immutables, but I guess these uses are hidden so it doesn't 
> really affect anything by doing the cast - but it seems sloppy when 
> looking at the code without looking everywhere to see how they are 
> used (which I suppose is short-hand for "someone in the future might 
> mess up because it's not obvious that these can't deal with any other 
> type").
>
> TransformHelper.java, line 108 & 112 - [formatting] perhaps move the 
> m[xyz],tx to the next line for consistency between the two (like 68 & 
> 76)?
>
> The implementation of ImmutableTransform copies a fair bit of 
> internals from Affine, but it could just wrap an Affine and save 
> itself all of that work.  Arguably, we could make it a super-class of 
> Affine and then it would own all of that code and Affine would 
> subclass only for the modification methods, but we'd either have to 
> expose it as a public class or have an unexplained inaccessible 
> superclass on Affine...? (Also, it's not clear if public methods on a 
> non-public superclass of Affine would be visible through Affine so 
> Affine would either have to have forwarding methods or we'd have to 
> make the super class public).
>
> On the other hand, what would be wrong with having a public BaseAffine 
> that was Affine without its modification methods, and having Affine 
> subclass it?  It would be effectively Immutable, but using the word 
> Immutable in its name might sound like a promise which would be 
> violated by its subclass - better to leave that property implied by 
> its lack of mutating methods and possibly mentioning the intent in the 
> javadoc...
>
>             ...jim
>
> On 5/3/2016 11:24 PM, Chien Yang wrote:
>> Kevin and Jim,
>>
>> Please review the proposed fix:
>>
>> Webrev: https://bugs.openjdk.java.net/browse/JDK-8155762
>> JIRA: http://cr.openjdk.java.net/~ckyang/JDK-8155762/webrev.00/
>>
>> Thanks,
>> - Chien