[webkit-reviews] review denied: [Bug 133609] CSS JIT: Reflect parent fragment's relations to sub fragments : [Attachment 232667] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Sat Jun 7 16:43:53 PDT 2014
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?
More information about the webkit-reviews
mailing list