RFR: JDK-8292276 : Add named colors from CSS Color Module Level 4 [v35]

[Phil Race]

On Thu, 17 Aug 2023 18:59:40 GMT, Phil Race <prr at openjdk.org> wrote:

>> As mentioned earlier in this review, you need to absorb the changes in 
>> https://github.com/openjdk/jdk/pull/10317/
>> into THIS PR and withdraw 10317.
>
>> @prrace
>> 
>> Do I absorb the changes in #15262 too ?
> 
> I hadn't even seen that one until you just mentioned it !
> I was puzzled because it was 8 months old .. but I then realised you've had it in draft all that time and just made it RFR a few days ago.
> 
> Having a third concurrent PR making changes to the same file is making things nearly insane.
> 
> The formal answer to your question is that
> (1) If these changes are unrelated in anyway to the other changes they can go separately, either before or after
> (2) If these changes require that the other changes be pushed first then they can go EITHER in the same PR or after
> (3)  if these changes are interdependent, meaning they cannot go before the other change and the other change also depends on them, then they should all be in one PR
> 
> If its (1) or (2) then I suggest we completely ignore this new PR until after the issues in the first one are pushed.

> @prrace and @aivanov-jdk #15262, #10317 and #9825 (this PR) are independant. That's why I planed to treat them separatly. It is also easier to test performance.

Not sure I see that to be the case.

> 
> Yes, all affect the result of the `stringToColor` method but they are unrealated except for the stringToColor comments which need to be updated.

They are not just "comments", they are Java SE API specification.
If you push code that does anything different,  that's a problem - it can't wait to be "updated in a later fix".

 
> `stringToColor` is public only in `StyleSheet.java` but not in `CSS.java`. 

But the one in Stylesheet is merely a wrapper around the one in CSS
    public Color stringToColor(String string) {
        return CSS.stringToColor(string);
    }

Basically at the very least you need to merge the two we asked you to merge.
I'll requote below what Alexey and I said in Feb
No one has looked at it enough to decide about the other one yet.

===
This specification change seems to be dependent on the change in implementation proposed in
https://github.com/openjdk/jdk/pull/10317/
JDK-8293776 : Adds CSS 4 and 8 digits hex coded Color
And BTW if you implemented that change on its own, it would not be spec compliant.

I think it best to WITHDRAW that other PR and absorb it in this PR

Also you should in the doc include the spec link.
spec limits what it will convert. I think

Initially, it looked as if handling these issues separately was a good idea. Now that we know that the specification of a public needs updating, which requires a CSR, it is better to have everything ready: named colors, hex-parsers, rgb() and rgba() parsers. The spec refers to all the cases. If one is missing, the method is not compliant to its specification.

Amending the implementation one by one would require updating the spec and reviewing the CSR. Thus, having everything ready in one larger PR makes more sense

=====

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

PR Comment: https://git.openjdk.org/jdk/pull/9825#issuecomment-1683052247