On Wed, 19 Feb 2025 22:54:15 GMT, Harshitha Onkar <hon...@openjdk.org> wrote:
>> Built-in Profiles are singleton objects and if the user happens to modify
>> this shared profile object via setData() then the modified version of the
>> profile is returned each time the same built-in profile is requested via
>> getInstance().
>>
>> It is good to protect Built-in profiles from such direct modification by
>> adding BuiltIn profile check in `setData()` such that **only copies** of
>> Built-In profiles are allowed to be updated.
>>
>> With the proposed fix, if Built-In profile is updated using `.setData()` it
>> throws _**IAE - "BuiltIn profile cannot be modified"**_
>>
>> Fix consists of:
>> * `private boolean isBuiltIn = false` in ICC_Profile which is set to true
>> when BuiltIn profiles are created and false for non-builtIn profile.
>> * Converted BuiltInProfile from private interface to private static class
>> (with all the profile instances as static final)
>> * `isBuiltIn` flag is set to true when BuiltInProfile are loaded.
>> * JavaDoc update for setData() (CSR required)
>>
>> There are no restrictions on creating copies of BuiltIn profile and then
>> modifying it, but what is being restricted with this fix is - the direct
>> modification of the shared BuiltIn profile instance.
>>
>> Applications which need a modified version of the ICC Profile should instead
>> do the following:
>>
>>
>> byte[] profileData = ICC_Profile.getData() // get the byte array
>> represtation of BuiltIn- profile
>> ICCProfile newProfile = ICC_Profile.getInstance(profileData) // create a new
>> profile
>> newProfile.setData() // to modify and customize the profile
>>
>>
>> Following existing tests are modified to update a copy of Built-In profile.
>>
>> - java/awt/color/ICC_Profile/SetHeaderInfo.java
>> - java/awt/color/ICC_ProfileSetNullDataTest.java
>> - sun/java2d/cmm/ProfileOp/SetDataTest.java
>
> Harshitha Onkar has updated the pull request incrementally with one
> additional commit since the last revision:
>
> javadoc change
Changes requested by aivanov (Reviewer).
src/java.desktop/share/classes/java/awt/color/ICC_Profile.java line 116:
> 114: * BuiltInProfile.
> 115: */
> 116: private boolean isBuiltIn = false;
Can't this field be `final` and be set in constructor of the class?
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.
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.
src/java.desktop/share/classes/java/awt/color/ICC_Profile.java line 1172:
> 1170: public void setData(int tagSignature, byte[] tagData) {
> 1171: if (isBuiltIn) {
> 1172: throw new IllegalArgumentException("BuiltIn profile"
Suggestion:
throw new IllegalArgumentException("Built-in profile"
-------------
PR Review: https://git.openjdk.org/jdk/pull/23606#pullrequestreview-2630072442
PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r1963699390
PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r1963690614
PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r1963694920
PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r1963695795
