Bug 133609 - CSS JIT: Reflect parent fragment's relations to sub fragments
Summary: CSS JIT: Reflect parent fragment's relations to sub fragments
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-06-07 12:26 PDT by Yusuke Suzuki
Modified: 2014-06-24 17:41 PDT (History)
8 users (show)

See Also:


Attachments
Patch (2.89 KB, patch)
2014-06-07 12:27 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (12.41 KB, patch)
2014-06-08 01:27 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 (631.75 KB, application/zip)
2014-06-08 03:31 PDT, Build Bot
no flags Details
Patch (16.83 KB, patch)
2014-06-24 14:17 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (17.10 KB, patch)
2014-06-24 15:07 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2014-06-07 12:26:41 PDT
Since some predicates use relationToRightFragment, reflecting parent fragment's relations to sub fragments is needed.
Comment 1 Yusuke Suzuki 2014-06-07 12:27:58 PDT
Created attachment 232667 [details]
Patch
Comment 2 WebKit Commit Bot 2014-06-07 12:29:51 PDT
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.
Comment 3 Yusuke Suzuki 2014-06-07 12:30:09 PDT
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.
Comment 4 Yusuke Suzuki 2014-06-07 12:30:55 PDT
Some tests are needed. I'll add it later.
Comment 5 Yusuke Suzuki 2014-06-07 12:37:16 PDT
Hmm, now I have no idea about how to test it.
I need to investigate it more.
Comment 6 Benjamin Poulain 2014-06-07 16:44:17 PDT
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?
Comment 7 Yusuke Suzuki 2014-06-08 01:24:09 PDT
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.
Comment 8 Yusuke Suzuki 2014-06-08 01:27:02 PDT
Created attachment 232682 [details]
Patch
Comment 9 Build Bot 2014-06-08 03:31:17 PDT
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
Comment 10 Build Bot 2014-06-08 03:31:20 PDT
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
Comment 11 Yusuke Suzuki 2014-06-08 10:33:06 PDT
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!
Comment 12 Yusuke Suzuki 2014-06-24 14:17:43 PDT
Created attachment 233740 [details]
Patch
Comment 13 Benjamin Poulain 2014-06-24 14:55:12 PDT
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()?
Comment 14 Yusuke Suzuki 2014-06-24 15:07:01 PDT
Created attachment 233747 [details]
Patch
Comment 15 WebKit Commit Bot 2014-06-24 15:57:06 PDT
Comment on attachment 233747 [details]
Patch

Clearing flags on attachment: 233747

Committed r170398: <http://trac.webkit.org/changeset/170398>
Comment 16 WebKit Commit Bot 2014-06-24 15:57:12 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 Ryosuke Niwa 2014-06-24 17:22:13 PDT
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
Comment 18 Benjamin Poulain 2014-06-24 17:41:24 PDT
(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.