RFR: 8299333: Unify exceptions used by all variants of ICC_Profile.getInstance(null) [v2]

[Phil Race]

On Fri, 20 Jan 2023 20:20:32 GMT, Sergey Bylokhov <serb at openjdk.org> wrote:

>> I tried to clean up the documentation in the `java.awt.color` package and specify how the `null` parameters are handled here and there. But it looks like the `ICC_profile` class is too specific so I would like to discuss it separately.
>> 
>> The ICC_Profile has these methods:
>>  * `getInstance(int)`      - always throws specified `IllegalArgumentException` on the wrong constant
>>  * `getInstance(byte[])` - always throws specified `IllegalArgumentException` on `null`
>>  * `getInstance(String fileName)` - always throws unspecified `NullPointerException` on `null`
>>  * `getInstance(InputStream)`      - thrown unspecified `NullPointerException` before [JDK-8227816](https://github.com/openjdk/jdk/commit/af20c6b9c4a3a21c1e2c7f56721e1077e7d8f388) and now throws specified but misleading `IOException: Stream closed` exception on `null`
>> 
>> So we have 3 methods that can get null as a parameter and each throw a different exception.
>> 
>> Note that all of the specs for the methods above have this part:
>> 
>>> @throws IllegalArgumentException If the stream does not contain valid ICC Profile data
>> 
>> So I have an idea to change `getInstance(String)` and `getInstance(InputStream)` to throw `IllegalArgumentException`  even if I personally prefer NPE to be thrown.
>> 
>> From another point of view the `ICC_Profile` profile has other methods:
>>  * `write(String fileName)` - always throws unspecified `NullPointerException` on `null`
>>  * `write(OutputStream) `  - always throws unspecified `NullPointerException` on `null`
>> 
>> I am not sure we can throw `IllegalArgumentException` from the `write` methods, so the specification for `getInstance(String)` and `getInstance(InputStream)` could be updated to throw NPE on null same as related `write`.
>> 
>> Thoughts?
>
> Sergey Bylokhov has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - Unify pairs for filenames and streams
>  - Revert "8299333: Unify used exceptions by all variants of ICC_Profile.getInstance(null)"
>    
>    This reverts commit db5cfdc280c00e38949469f9bc6fcb3d06a12716.

I finally got around to reviewing the CSR. You can now finalize it.

src/java.desktop/share/classes/java/awt/color/ICC_Profile.java line 869:

> 867:      */
> 868:     public static ICC_Profile getInstance(String fileName) throws IOException {
> 869:         if (fileName == null) {

The reasons given for IOException seems more appropriate since a null file cannot be opened but I'm sure you  don't think that is the right exception either
I think it might be best to update the spec along with this change to be clear.

src/java.desktop/share/classes/java/awt/color/ICC_Profile.java line 902:

> 900:      */
> 901:     public static ICC_Profile getInstance(InputStream s) throws IOException {
> 902:         if (s == null) {

Same comment here. We should make it clear in the spec.

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

Marked as reviewed by prr (Reviewer).

PR: https://git.openjdk.org/jdk/pull/11784