On Wed, 2 Apr 2025 22:36:13 GMT, Harshitha Onkar <[email protected]> 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"**_
>>
>> 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
>> representation 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:
>
> single case type
Marked as reviewed by aivanov (Reviewer).
test/jdk/java/awt/color/ICC_Profile/BuiltInProfileCheck/BuiltInProfileCheck.java
line 92:
> 90:
> 91: private static void prepareTestProfile(String testCase,
> 92: boolean isBuiltIn, int cs) {
I'd probably return the resulting profile to run test on, it will make data
flow explicit. However, it'd be more verbose… to pass the test profile.
So, the current approach is sensible.
Could you write a comment that `prepareTestProfile` stores the profile to test
in `testProfile`? It could be a javadoc for the `testProfile` field.
-------------
PR Review: https://git.openjdk.org/jdk/pull/23606#pullrequestreview-2754849213
PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r2036165465
