On Wed, 16 Aug 2023 14:06:16 GMT, John Hendrikx <[email protected]> wrote:
>> modules/javafx.graphics/src/main/java/javafx/css/Match.java line 1:
>>
>>> 1: /*
>>
>> The compare method seems wrong. I think it should delegate to
>> `Integer.compare`.
>>
>> `specificity` should also be made `private`.
>
> These were not changed as part of this PR, although I agree on both counts.
I should add, even though there are some edge cases where the compare method
could fail in `Match`, it will work correctly in practice as `specificity` is
always a positive number.
specificity = (idCount << 8) | (styleClassCount << 4) | nPseudoClasses;
I do question how this is calculated though. It seems to make assumptions that
`styleClassCount` and the number of pseudo classes never exceeds 16 -- I would
agree that having 16 style classes / pseudo classes in a match is on the high
side, but certainly not so high that it could never happen.
It seems to be a bit of a premature optimization, since `Match` retains all the
information that was used to calculate `specificity`, and so they could have
instead have compared `idCount`, `styleClassCount` and `pseudoClasses.size()`
in turn...
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1076#discussion_r1296004826
