RFR: JDK-8346465 : Add a check in setData() to restrict the update of Built-In ICC_Profiles [v5]

[Alexey Ivanov]

On Thu, 20 Feb 2025 19:38:29 GMT, Harshitha Onkar <honkar at openjdk.org> wrote:

>>> Can't this field be final and be set in constructor of the class?
>>> > Indeed, it _can be_, and I prefer this design. This way, the `isBuiltIn` field can't be changed after the object is constructed. I think it's a cleaner design.
>> 
>> A cleaner diff between your latest commit and my commit on top: https://github.com/openjdk/jdk/compare/8da82c10b5be3b92655abef95fd5be0c8b6eaa00..6729625f39c0dc47cd2588906b1793611996ca10
>> 
>> This link shouldn't become invalid after either of us removes the corresponding branches from our forks.
>
>> Indeed, it can be, and I prefer this design. This way, the isBuiltIn field can't be changed after the object is constructed. I think it's a cleaner design.
> 
> I agree, the approach you mentioned does have merits. After evaluating both approaches setting in static block was preferred over the constructor for the following reason.
> Setting it via constructor meant that we could mark the profile as Built-In only when it was constructed using ProfileDeferralInfo and modifying all the constructors - ICC_Profile, ICC_ProfileRGB, ICC_ProfileGray whereas setting it in static block meant minimal changes to existing code.

I don't get the reason why it's bad to modify the constructors.

At this time, all the built-in profiles are created with `ProfileDeferralInfo`.

If `ProfileDeferralInfo` is used to create only built-in profiles, then modifying `ICC_Profile(ProfileDeferralInfo pdi)` is enough:


    ICC_Profile(ProfileDeferralInfo pdi) {
        deferralInfo = pdi;
        isBuiltIn = true;
    }


And this is a *minimal* change.

When the `isBuiltIn` is final, it's much easier to find where the value gets set.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r1964330041