RFR: JDK-8346465 : Add a check in setData() to restrict the update of Built-In ICC_Profiles [v5]

[Alexey Ivanov]

On Thu, 20 Feb 2025 18:20:52 GMT, Harshitha Onkar <honkar at openjdk.org> wrote:

>> src/java.desktop/share/classes/java/awt/color/ICC_Profile.java line 1154:
>> 
>>> 1152:      * This method is useful for advanced applications which need to access
>>> 1153:      * profile data directly. Only non-built-in, application provided profiles
>>> 1154:      * should be updated using this method.
>> 
>> What you mean is that only non-built-in profiles *can* be updated. The fix makes it impossible to update built-in profiles.
>
> I'll be inverting this statement to state what is NOT allowed rather than what is allowed by .setData() as below. Does the following sound better?
> 
> 
>      * Note: JDK built-in ICC Profiles cannot be updated using this method
>      * as it will result in IAE. JDK built-in profiles are those obtained by
>      * {@code ICC_Profile.getInstance(int colorSpaceID)} where colorSpaceID
>      * is one of the following:
>      * {@link ColorSpace#CS_sRGB}, {@link ColorSpace#CS_LINEAR_RGB},
>      * {@link ColorSpace#CS_PYCC}, {@link ColorSpace#CS_GRAY} or
>      * {@link ColorSpace#CS_CIEXYZ}

This is clearer.

>> src/java.desktop/share/classes/java/awt/color/ICC_Profile.java line 1164:
>> 
>>> 1162:      *         array can not be interpreted as valid tag data, corresponding to
>>> 1163:      *         the {@code tagSignature}
>>> 1164:      * @throws IllegalArgumentException if this is a profile for one of the
>> 
>> `IllegalStateException` better describes the reason: the argument to the method can be perfectly valid, but the internal state of the object doesn't allow modifications.
>
> @aivanov-jdk 
> 
> _IllegalStateException  - Signals that a method has been invoked at an **illegal or inappropriate time.**_  
> Since IllegalStateException is thrown to indicate more of an unstable state of object, it may not be what we want here. The exception is to be thrown when the ICC_Profile object invoking .setData() is JDK Built-in profile and IIlegalArgumentException more closely match this case.

The javadoc for `IllegalArgumentException` says, <q cite="https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/lang/IllegalArgumentException.html">Thrown to indicate that a method has been passed <em>an illegal or inappropriate argument</em>.</q> (Emphasis mine.)

Getting an `IllegalArgumentException` for a valid argument would be confusing.

`IllegalStateException`, as you said, “signals that a method has been invoked at an <em>illegal or inappropriate</em> time”. It's exactly the state of the object: a *built-in* color profile cannot be modified.

Although, there's no time where such a modification will be allowed, I still think `IllegalStateException` better conveys the meaning: changes to built-in profiles are *never* allowed.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r1964136111
PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r1964135080