[webkit-reviews] review denied: [Bug 133609] CSS JIT: Reflect parent fragment's relations to sub fragments : [Attachment 232667] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Yusuke Suzuki
<utatane.tea at gmail.com>'s request for review:
Bug 133609: CSS JIT: Reflect parent fragment's relations to sub fragments
https://bugs.webkit.org/show_bug.cgi?id=133609

Attachment 232667: Patch
https://bugs.webkit.org/attachment.cgi?id=232667&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=232667&action=review


Good catch!

It is a shame not a single test caught that bug.

I think this needs test so that such issue never happen again. The filters
:active and :hover are probably harder to test for this. The filters
":first-child", ":last-child" and ":nth-child" would be easier to test.

Something like this might work for testing:
-have a bunch of sibling elements.
-use one of the functional pseudo class on the rightmost selector with one of
the marking filters (let's say :first-child for example).
-test the style.
-move the order of the elements programmatically.
-test the style again.

If the elements share the style because "flagIsUnique" is not set, you should
be able to get the wrong style.

> Source/WebCore/ChangeLog:12
> +	   No new tests (OOPS!).

We you have no test, you need to remove this line to be able to land a patch.

>> Source/WebCore/cssjit/SelectorCompiler.cpp:618
>> +	    // Some non-backtrack-related predicates use
relationToRightFragment information to decide whether
checkingContext->elementStyle should be refered.
>> +	    // To make it works correctly, Reflect fragment's relations to sub
fragments.
> 
> For example, `generateElementIsActive` uses it.

This works but I am afraid it is not future proof. It is very likely that
functional pseudo class will support relation in the future.
Let's add a boolean on SelectorFragment instead. Maybe "isRightmost" or
something like that?