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

[Phil Race]

On Thu, 16 Dec 2021 18:33:37 GMT, Jeremy <duke at openjdk.java.net> wrote:

>> This removes code that relied on consulting the Bezier control points to calculate the Rectangle2D bounding box. Instead it's pretty straight-forward to convert the Bezier control points into the x & y parametric equations. At their most complex these equations are cubic polynomials, so calculating their extrema is just a matter of applying the quadratic formula to calculate their extrema. (Or in path segments that are quadratic/linear/constant: we do even less work.)
>> 
>> The bug writeup indicated they wanted Path2D#getBounds2D() to be more accurate/concise. They didn't explicitly say they wanted CubicCurve2D and QuadCurve2D to become more accurate too. But a preexisting unit test failed when Path2D#getBounds2D() was updated and those other classes weren't. At this point I considered either:
>> A. Updating CubicCurve2D and QuadCurve2D to use the new more accurate getBounds2D() or
>> B. Updating the unit test to forgive the discrepancy.
>> 
>> I chose A. Which might technically be seen as scope creep, but it feels like a more holistic/better approach.
>> 
>> Other shapes in java.awt.geom should not require updating, because they already identify concise bounds.
>> 
>> This also includes a new unit test (in Path2D/UnitTest.java) that fails without the changes in this commit.
>
> 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
>   
>   This is a second follow-up response to prrace's code review feedback about method modifiers.
>   
>   This commit more carefully preserves the getBounds2D() method modifiers for all 3 classes: the Path2D.Double, the Path2D.Float, and the Path2D itself.
>   
>   It is possible (if unlikely) that someone previously extended the Path2D class and overrode getBounds2D(), because the Path2D#getBounds2D() method was *not* final. So with this commit: any such existing code will not break. Meanwhile the subclasses (Double and Float) preserve their previous modifiers (final, synchronized).
>   
>   This is in response to prrace's code review:
>   
>   >    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.

I still recommend a CSR since
1) Although "compatible", this visibly moves the specs for getBounds2D() from copies on the Float and Double nested sub-classes to the enclosing parent class
2) Although in the spirit of what getBounds2D() is supposed to do  .. it actually hasn't for over 20 years. We like to document such behavioural changes via a CSR.

Actually regarding (1) there's also a real change there too since now The Float subclass will return a Rectangle2D.Double .. I am not sure that we should actually do that.

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

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