On Mon, 10 Mar 2025 23:22:36 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"**_
>> 
>> 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:
> 
>   builtIn converted to transient, tests updated

Changes requested by aivanov (Reviewer).

src/java.desktop/share/classes/java/awt/color/ICC_Profile.java line 1174:

> 1172:         if (builtIn) {
> 1173:             throw new IllegalArgumentException("Built-in profile"
> 1174:                                                + " cannot be modified");

Is it acceptable to overshoot 80-column limit for 6 characters without wrapping 
the line?

test/jdk/java/awt/color/ICC_Profile/BuiltInProfileCheck.java line 69:

> 67:                 throw new RuntimeException("Test Failed! IAE NOT 
> thrown.");
> 68:             } catch (IllegalArgumentException iae) {
> 69:                 System.out.println("IAE expected: " + iae.getMessage());

I'm for keeping the previous version which verified the exception message — 
otherwise, how can we distinguish `IllegalArgumentException` for illegal 
arguments?

If we used another exception type, there wouldn't be the need to check on the 
message.

test/jdk/java/awt/color/ICC_ProfileSetNullDataTest.java line 41:

> 39:             ColorSpace.CS_CIEXYZ, "CS_CIEXYZ",
> 40:             ColorSpace.CS_LINEAR_RGB, "CS_LINEAR_RGB"
> 41:     ));

Suggestion:

    private static final Map<Integer, String> colorSpace = Map.of(
            ColorSpace.CS_sRGB, "CS_sRGB",
            ColorSpace.CS_PYCC, "CS_PYCC",
            ColorSpace.CS_GRAY, "CS_GRAY",
            ColorSpace.CS_CIEXYZ, "CS_CIEXYZ",
            ColorSpace.CS_LINEAR_RGB, "CS_LINEAR_RGB"
    );


Printing the `int` value of a color space could be enough, although a readable 
name is better.

test/jdk/java/awt/color/ICC_ProfileSetNullDataTest.java line 49:

> 47:             ICC_Profile profile = 
> ICC_Profile.getInstance(builtInProfile.getData());
> 48:             try {
> 49:                 profile.setData(ICC_Profile.icSigCmykData, tagData);

Suggestion:

                profile.setData(ICC_Profile.icSigCmykData, null);

Should this be simplified?

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

PR Review: https://git.openjdk.org/jdk/pull/23606#pullrequestreview-2674550615
PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r1989502914
PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r1989287031
PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r1989489808
PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r1989491425

Reply via email to