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

[Sergey Bylokhov]

On Wed, 5 Mar 2025 21:20:11 GMT, Harshitha Onkar <honkar at openjdk.org> wrote:

>>> can we just ignore it instead and did not use suspicion IllegalArgumentException for correct parameters? or change the type to something unrelated to "..ArgumentException"?
>> 
>> This has been [my suggestion](https://github.com/openjdk/jdk/pull/23606#discussion_r1968073992) since the start of this code review.
>> 
>>> The silently do nothing option was considered, but if you do that, then you have no easy way of knowing if it worked.  
>>> Tests may pass spuriously, or fail later for the wrong reasons. So a worse choice.
>> 
>> I agree, an exception should be thrown if modification isn't allowed.
>> 
>>> And in all my searching of uses of this API it is (1) tests in the JDK itself and (2) a couple of libraries that are targeting specific known profiles with issues and are fixed up - so never applied to built-in profiles.
>> 
>> Therefore we can throw <code>Illegal<i>State</i>Exception</code> instead of <code>Illegal<i>Argument</i>Exception</code>.
>> 
>> Quoting here [Harshitha's reply](https://github.com/openjdk/jdk/pull/23606#discussion_r1968263836):
>> 
>>> As Phil mentioned earlier, the already documented exception IAE for .setData() outweighs introducing a new exception.
>> 
>> Using the exception which clearly describes the reason why it was thrown outweighs re-using an already documented exception…
>> 
>>> Using an existing exception would mean less disruption to any existing applications using the Java API.
>> 
>> …especially because the search for usages of `setData` revealed that there are no usages of `setData` which will be affected by the proposed change.
>> 
>> If I'm not mistaken, the search also showed that no usages of `setData` handle `IllegalArgumentException` — thus apps will be equally unprepared to any type of an unchecked exception.
>
> @aivanov-jdk @mrserb 
> There are two other exceptions that may be better suited that ISE in this case - [UnsupportedOperationException ](https://docs.oracle.com/en/java/javase/23/docs/api/java.base/java/lang/UnsupportedOperationException.html) and [ProfileDataException](https://docs.oracle.com/en/java/javase/21/docs/api/java.desktop/java/awt/color/ProfileDataException.html) but still IAE was chosen oven them as it is existing and documented exception.
> 
>> If I'm not mistaken, the search also showed that no usages of setData handle IllegalArgumentException — thus apps will be equally unprepared to any type of an unchecked exception.
> 
> Reasonable point.

I'm just not sure how this should be handled from the application's point of view. The app may obtain a profile from some place and use it for some images or pixels. Then, if the app wants to tweak the rendering intent for some reason, what should it do?

 - Clone the profile and then change the intent?
 - Change the intent, and if an exception occurs, clone the profile and try to change the intent again?
 - Skip changing the intent completely?

I remember we discussed this issue during the review of [this commit](https://github.com/openjdk/jdk/pull/3037/files#diff-3dd53a33889801159f43dbb990ba033066bdabaed71bbc7254f58331d3898d69R39), and the conclusion was that it was too late to change it and break apps. It wasn’t considered very important since the "appcontext was withdrawn". So, why are we deciding to change it now?

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

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