On Sat, 1 Mar 2025 01:11: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"**_
>>
>> 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:
>
> doc update
Changes requested by aivanov (Reviewer).
src/java.desktop/share/classes/java/awt/color/ICC_Profile.java line 113:
> 111: /**
> 112: * Set to true for {@code BuiltInProfile}, false otherwise.
> 113: * This check is used in {@link #setData(int, byte[])} to prevent
> modifying
Suggestion:
* Set to {@code true} for {@code BuiltInProfile}, {@code false} otherwise.
* This check is used in {@link #setData(int, byte[])} to prevent modifying
[This
suggestion](https://github.com/openjdk/jdk/pull/23606#discussion_r1969536537)
isn't fully applied, yet it's marked as resolved.
The [javadoc style
guide](https://www.oracle.com/technical-resources/articles/java/javadoc-tool.html#styleguide)
recommends, <q
cite="https://www.oracle.com/technical-resources/articles/java/javadoc-tool.html#styleguide";>Use
`<code>` style for keywords and names.</q> Therefore both `true` and `false`
should be marked up with `{@code}`.
src/java.desktop/share/classes/java/awt/color/ICC_Profile.java line 132:
> 130:
> 131: ICC_Profile LRGB = new ICC_ProfileRGB(new ProfileDeferralInfo(
> 132: "LINEAR_RGB.pf", ColorSpace.TYPE_RGB, 3, CLASS_DISPLAY));
There should be no additional space added, all these lines in `BuiltInProfile`
should remain unmodified when compared to the `master` branch.
src/java.desktop/share/classes/java/awt/color/ICC_Profile.java line 1168:
> 1166: * the {@code tagSignature}
> 1167: * @throws IllegalArgumentException if this is a built-in profile
> for one
> 1168: * of the pre-defined ColorSpaces, i.e. those which can be
> obtained
Suggestion:
* of the pre-defined color spaces, i.e. those which can be obtained
Here, you refer to _“color spaces”_ as the concept rather than the
[`ColorSpace`](https://docs.oracle.com/en/java/javase/21/docs/api/java.desktop/java/awt/color/ColorSpace.html)
class, therefore regular capitalisation and spelling applies.
-------------
PR Review: https://git.openjdk.org/jdk/pull/23606#pullrequestreview-2657212692
PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r1979306472
PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r1979320565
PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r1979315193
