RFR: 8176501: Method Shape.getBounds2D() incorrectly includes Bezier control points in bounding box [v10]

[Laurent Bourgès]

On Wed, 15 Dec 2021 23:49:24 GMT, Phil Race <prr at openjdk.org> wrote:

>> Jeremy has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   8176501: Method Shape.getBounds2D() incorrectly includes Bezier control points in bounding box
>>   
>>   Addressing code review feedback: "Use BigDecomal.setScale(40) to ascertain precision."
>>   
>>   The geom.* unit tests passed before & after this change.
>>   
>>   https://github.com/openjdk/jdk/pull/6227#discussion_r752908494
>
> There seems to be a number of things to discuss aside from the maths
> 
> - You are changing the signature of public API
>   src/java.desktop/share/classes/java/awt/geom/Path2D.java
> public final synchronized Rectangle2D getBounds2D() => public Rectangle2D getBounds2D() {
> 
> So no longer final, and no longer synchronized.
> This means a CSR is required and we need to think about it .. the intention was that the subclass not over-ride.
> And why remove synchronized ? I am fairly sure it was there to make sure no one was mutating the Path whilst
> bounds are being calculated.
> And you are using getPathIterator(AffineTransform) and the docs for that say it isn't thread safe.
> So I think this implementation needs to be thought about very carefully.
> 
> - You are adding a new public static method
> 
> public static Rectangle2D getBounds2D(final PathIterator pi);
> 
> Is this really necessary ? It is just for the benefit of the public API caller so can be package private.
> 
> - For me it doesn't build because of a doclint error
> 
> src/java.desktop/share/classes/java/awt/geom/Path2D.java:2102: warning: no @param for pi
>     public static Rectangle2D getBounds2D(final PathIterator pi) {
>                               ^
> error: warnings found and -Werror specified
> 
> Fixing that I find that the updated UnitTest.java fail with a build with these changes :-
> 
> % java UnitTest
> Exception in thread "main" java.lang.RuntimeException: the shape must not intersect anything above its bounds
>     at UnitTest.testGetBounds2D(UnitTest.java:1396)
>     at UnitTest.testBounds(UnitTest.java:1302)
>     at UnitTest.test(UnitTest.java:1175)
>     at UnitTest.main(UnitTest.java:1435)
> 
> Then we have some even more difficult questions.
> The control points may not be within the bounds of the curve which is of course the premise of the bug report.
> The specification (javadoc) isn't very forthcoming on whether it is allowed to not include them .. ie is it expected they should be ?
> And it appears we have JCK tests which expect they ARE included. So as it is this change would not pass the existing TCK/JCK. 
> Which means we'd have to be 100% sure about this change in behaviour and be able to justify it.
> FWIW unless the spec explicitly says they are, in some place I've overlooked,
> then I'd say its a quality-of-implementation thing as to how tight the bounds are, and the new behaviour is allowed by spec.
> Applications however might still find this a behaviourally incompatible change .. and could potentially see undesirable consequences -
> even as other applications see better consequences.
> So perhaps new API that defines tighter bounds  might be  the way to address this .. but I am far from being sure about that.

@prrace could you say on CSR or not ?

-------------

PR: https://git.openjdk.java.net/jdk/pull/6227