Bug 133609 – CSS JIT: Reflect parent fragment's relations to sub fragments

• Component: CSS
• Status: RESOLVED FIXED

[Yusuke Suzuki]

Since some predicates use relationToRightFragment, reflecting parent fragment's relations to sub fragments is needed.

[Yusuke Suzuki]

Created attachment 232667 [details]
Patch

[WebKit Commit Bot]

Attachment 232667 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:12:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Comment on attachment 232667 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=232667&action=review

> Source/WebCore/cssjit/SelectorCompiler.cpp:618
> +        // To make it works correctly, Reflect fragment's relations to sub fragments.

For example, `generateElementIsActive` uses it.

[Yusuke Suzuki]

Some tests are needed. I'll add it later.

[Yusuke Suzuki]

Hmm, now I have no idea about how to test it.
I need to investigate it more.

[Benjamin Poulain]

Comment on attachment 232667 [details]
Patch

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?

[Yusuke Suzuki]

Comment on attachment 232667 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=232667&action=review

>> 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.

OK, so in the meantime, remove this.

>>> Source/WebCore/cssjit/SelectorCompiler.cpp:618
>>> +        // 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?

That's nice. So I'll add flag to constructFragments and field to SelectorFragment.
It represents that where this fragment is in root fragment list.

[Yusuke Suzuki]

Created attachment 232682 [details]
Patch

[Build Bot]

Comment on attachment 232682 [details]
Patch

Attachment 232682 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6333817916751872

New failing tests:
media/W3C/video/networkState/networkState_during_loadstart.html

[Build Bot]

Created attachment 232683 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Yusuke Suzuki]

Ah, I've missed your overall comment.

> 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.

I'll add tests!

[Yusuke Suzuki]

Created attachment 233740 [details]
Patch

[Benjamin Poulain]

Comment on attachment 233740 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=233740&action=review

> Source/WebCore/cssjit/SelectorCompiler.cpp:345
> +static inline bool isRightmostInRootFragments(const SelectorFragment& fragment)
> +{
> +    return fragment.relationToRightFragment == FragmentRelation::Rightmost && fragment.positionInRootFragments == FragmentPositionInRootFragments::Rightmost;
> +}

I would rename this by how it is used for code generation.
Maybe: shouldUseRenderStyleFromCheckingContext()?

[Yusuke Suzuki]

Created attachment 233747 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 233747 [details]
Patch

Clearing flags on attachment: 233747

Committed r170398: <http://trac.webkit.org/changeset/170398>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

It looks like this patch broke a bunch of transitions tests:
http://build.webkit.org/results/Apple%20Mavericks%20Release%20WK1%20(Tests)/r170398%20(6904)/results.html

[Benjamin Poulain]

(In reply to comment #17)
> It looks like this patch broke a bunch of transitions tests:
> http://build.webkit.org/results/Apple%20Mavericks%20Release%20WK1%20(Tests)/r170398%20(6904)/results.html

I looked into those tests, I believe it is unrelated.