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