On Mon, 3 Mar 2025 18:01:24 GMT, Phil Race <p...@openjdk.org> wrote:
>> src/java.desktop/share/classes/java/awt/color/ICC_Profile.java line 1169:
>>
>>> 1167: * @throws IllegalArgumentException if this is a built-in profile
>>> for one
>>> 1168: * of the pre-defined ColorSpaces, i.e. those which can
>>> be obtained
>>> 1169: * by calling {@code ICC_Profile.getInstance(int
>>> colorSpaceID)}
>>
>> can we just ignore it instead and did not use suspicion
>> IllegalArgumentException for correct parameters? or change the type to
>> something unrelated to "..ArgumentException"?
>
> 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.
> 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.
> 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.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r1979357419
