On Fri, 21 Feb 2025 00:50:45 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"**_
>>
>> 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 update
Changes requested by aivanov (Reviewer).
src/java.desktop/share/classes/java/awt/color/ICC_Profile.java line 1163:
> 1161: * {@link ColorSpace#CS_PYCC}, {@link ColorSpace#CS_GRAY} or
> 1162: * {@link ColorSpace#CS_CIEXYZ}.
> 1163: * </p>
Suggestion:
* <p>
* Note: JDK built-in ICC Profiles cannot be updated using this method
* as it will result in {@code IllegalArgumentException}. JDK built-in
* profiles are those obtained by
* {@code ICC_Profile.getInstance(int colorSpaceID)}
* where {@code colorSpaceID} is one of the following:
* {@link ColorSpace#CS_sRGB}, {@link ColorSpace#CS_LINEAR_RGB},
* {@link ColorSpace#CS_PYCC}, {@link ColorSpace#CS_GRAY} or
* {@link ColorSpace#CS_CIEXYZ}.
* </p>
You should spell `IllegalArgumentException` fully, it explicitly lists the
exception thrown. And `colorSpaceID` should also be marked up with `{@code}`.
src/java.desktop/share/classes/java/awt/color/ICC_Profile.java line 1170:
> 1168: * @throws IllegalArgumentException if {@code tagSignature} is not a
> 1169: * signature as defined in the ICC specification.
> 1170: * @throws IllegalArgumentException if a content of the {@code
> tagData}
This is an existing issue, if it's an issue: should the text say *“**the**
content”* instead of *“**a** content”*? @prrace
src/java.desktop/share/classes/java/awt/color/ICC_Profile.java line 1175:
> 1173: * @throws IllegalArgumentException if this is a profile for one of
> the
> 1174: * built-in pre-defined ColorSpaces, i.e. those which can
> be obtained
> 1175: * by calling {@code ICC_Profile.getInstance(int
> colorSpaceID)}
Suggestion:
* @throws IllegalArgumentException if this is a built-in profile for one
of the
* pre-defined color spaces, i.e. those which can be obtained
* by calling {@code ICC_Profile.getInstance(int colorSpaceID)}
test/jdk/java/awt/color/ICC_Profile/BuiltInProfileCheck.java line 42:
> 40: System.out.println("CASE 1: Testing BuiltIn Profile");
> 41: updateProfile(true);
> 42: System.out.println("Passed \n");
Suggestion:
System.out.println("Passed\n");
test/jdk/java/awt/color/ICC_Profile/BuiltInProfileCheck.java line 51:
> 49: private static void updateProfile(boolean isBuiltIn) {
> 50: ICC_Profile builtInProfile =
> ICC_Profile.getInstance(ColorSpace.CS_sRGB);
> 51: //create a copy of the BuiltIn profile
Suggestion:
// Create a copy of the built-in profile
test/jdk/java/awt/color/ICC_Profile/BuiltInProfileCheck.java line 58:
> 56: byte[] headerData = iccProfile.getData(HEADER_TAG);
> 57: int index = PROFILE_CLASS_START_INDEX;
> 58: //set profile class to valid icSigInputClass = 0x73636E72
Suggestion:
// Set profile class to valid icSigInputClass = 0x73636E72
test/jdk/java/awt/color/ICC_Profile/BuiltInProfileCheck.java line 66:
> 64: if (isBuiltIn) {
> 65: try {
> 66: //try updating a BuiltIn Profile, the following stmt
> should throw IAE
Suggestion:
// Try updating a built-in profile, IAE is expected
test/jdk/java/awt/color/ICC_Profile/BuiltInProfileCheck.java line 68:
> 66: //try updating a BuiltIn Profile, the following stmt
> should throw IAE
> 67: iccProfile.setData(HEADER_TAG, headerData);
> 68: throw new RuntimeException("Test Failed ! IAE NOT
> thrown.");
Suggestion:
throw new RuntimeException("Test Failed! IAE NOT thrown.");
test/jdk/java/awt/color/ICC_Profile/BuiltInProfileCheck.java line 76:
> 74: }
> 75: } else {
> 76: //modifying custom profile should NOT throw IAE
Suggestion:
// Modifying custom profile should NOT throw IAE
test/jdk/java/awt/color/ICC_ProfileSetNullDataTest.java line 33:
> 31: */
> 32: public final class ICC_ProfileSetNullDataTest {
> 33: private static final int[] colorSpace = new int [] {
Suggestion:
private static final int[] colorSpace = new int[] {
test/jdk/java/awt/color/ICC_ProfileSetNullDataTest.java line 48:
> 46: } catch (IllegalArgumentException e) {
> 47: return;
> 48: }
The test won't run for **all** the cases if you after the first successful case
— use `continue`.
-------------
PR Review: https://git.openjdk.org/jdk/pull/23606#pullrequestreview-2637741457
PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r1968002547
PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r1968004976
PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r1968009066
PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r1968033857
PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r1968022912
PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r1968028128
PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r1968027136
PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r1968027393
PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r1968028624
PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r1968029468
PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r1968032947
