Bug 135242 – compile scrollbar pseudoclass css selectors

• Component: CSS
• Status: RESOLVED FIXED

[Alex Christensen]

This requires some changes to the css selector compiler that will allow compiling of other pseudoclasses.  I have a few questions:
1) Are the tests in LayoutTests/scrollbars sufficient?
2) Why are the scrollbar pseudoclasses separate in SelectorChecker::checkScrollbarPseudoClass and SelectorChecker::checkOne?  Should I do something similar in the compiler?
3) SelectorChecker::SelectorCheckingContext and SelectorCompiler::CheckingContext are starting to look the same.  Should they be rearranged to be the same class?  Is adding more things into SelectorCompiler::CheckingContext going to have a significant performance cost?

[Alex Christensen]

Created attachment 235436 [details]
Patch

[Build Bot]

Comment on attachment 235436 [details]
Patch

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

New failing tests:
media/track/add-and-remove-track.html

[Build Bot]

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

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

[Alex Christensen]

Created attachment 235446 [details]
Patch

[Alex Christensen]

Comment on attachment 235446 [details]
Patch

This needs to be redone after https://bugs.webkit.org/show_bug.cgi?id=134835
Additional tests need to be added because returning FunctionType::CannotMatchAnything doesn't make any tests fail :(

[Yusuke Suzuki]

(In reply to comment #5)
> (From update of attachment 235446 [details])
> This needs to be redone after https://bugs.webkit.org/show_bug.cgi?id=134835
> Additional tests need to be added because returning FunctionType::CannotMatchAnything doesn't make any tests fail :(

Now 134835[1] is landed[2] :)

[1]: https://bugs.webkit.org/show_bug.cgi?id=134835
[2]: http://trac.webkit.org/changeset/171588

[Alex Christensen]

Created attachment 235554 [details]
Patch

[Alex Christensen]

This latest patch is work in progress after Yusuke's patch.  I'm still missing something fundamental, but it's probably 80% of the way there.

[Build Bot]

Comment on attachment 235554 [details]
Patch

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

New failing tests:
media/media-fragments/TC0001.html

[Build Bot]

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

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

[Alex Christensen]

Created attachment 235607 [details]
Patch

[Alex Christensen]

This latest patch seems to work with everything in LayoutTests/scrollbars/overflow-scrollbar-combinations.html.  I think it's done except for adding many test cases.

[Martin Hock]

Comment on attachment 235607 [details]
Patch

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

> Source/WebCore/cssjit/SelectorCompiler.cpp:406
> +    case CSSSelector::PseudoClassWindowInactive:
> +        fragment.unoptimizedPseudoClassesWithContext.append(JSC::FunctionPtr(scrollbarMatchesWindowInactivePseudoClass<CheckingContext>));
> +        break;
> +    case CSSSelector::PseudoClassDisabled:
> +        fragment.unoptimizedPseudoClassesWithContext.append(JSC::FunctionPtr(scrollbarMatchesDisabledPseudoClass<CheckingContext>));
> +        break;
> +    case CSSSelector::PseudoClassEnabled:
> +        fragment.unoptimizedPseudoClassesWithContext.append(JSC::FunctionPtr(scrollbarMatchesEnabledPseudoClass<CheckingContext>));
> +        break;
> +    case CSSSelector::PseudoClassHorizontal:
> +        fragment.unoptimizedPseudoClassesWithContext.append(JSC::FunctionPtr(scrollbarMatchesHorizontalPseudoClass<CheckingContext>));
> +        break;
> +    case CSSSelector::PseudoClassVertical:
> +        fragment.unoptimizedPseudoClassesWithContext.append(JSC::FunctionPtr(scrollbarMatchesVerticalPseudoClass<CheckingContext>));
> +        break;
> +    case CSSSelector::PseudoClassDecrement:
> +        fragment.unoptimizedPseudoClassesWithContext.append(JSC::FunctionPtr(scrollbarMatchesDecrementPseudoClass<CheckingContext>));
> +        break;
> +    case CSSSelector::PseudoClassIncrement:
> +        fragment.unoptimizedPseudoClassesWithContext.append(JSC::FunctionPtr(scrollbarMatchesIncrementPseudoClass<CheckingContext>));
> +        break;
> +    case CSSSelector::PseudoClassStart:
> +        fragment.unoptimizedPseudoClassesWithContext.append(JSC::FunctionPtr(scrollbarMatchesStartPseudoClass<CheckingContext>));
> +        break;
> +    case CSSSelector::PseudoClassEnd:
> +        fragment.unoptimizedPseudoClassesWithContext.append(JSC::FunctionPtr(scrollbarMatchesEndPseudoClass<CheckingContext>));
> +        break;
> +    case CSSSelector::PseudoClassDoubleButton:
> +        fragment.unoptimizedPseudoClassesWithContext.append(JSC::FunctionPtr(scrollbarMatchesDoubleButtonPseudoClass<CheckingContext>));
> +        break;
> +    case CSSSelector::PseudoClassSingleButton:
> +        fragment.unoptimizedPseudoClassesWithContext.append(JSC::FunctionPtr(scrollbarMatchesSingleButtonPseudoClass<CheckingContext>));
> +        break;
> +    case CSSSelector::PseudoClassNoButton:
> +        fragment.unoptimizedPseudoClassesWithContext.append(JSC::FunctionPtr(scrollbarMatchesNoButtonPseudoClass<CheckingContext>));
> +        break;
> +    case CSSSelector::PseudoClassCornerPresent:
> +        fragment.unoptimizedPseudoClassesWithContext.append(JSC::FunctionPtr(scrollbarMatchesCornerPresentPseudoClass<CheckingContext>));
> +        break;
> +    case CSSSelector::PseudoClassActive:
> +        fragment.unoptimizedPseudoClassesWithContext.append(JSC::FunctionPtr(scrollbarMatchesActivePseudoClass<CheckingContext>));
> +        break;
> +    case CSSSelector::PseudoClassHover:
> +        fragment.unoptimizedPseudoClassesWithContext.append(JSC::FunctionPtr(scrollbarMatchesHoverPseudoClass<CheckingContext>));
> +        break;

This can be simplified by writing a function like the following (note, syntax may be a bit off, if so, apologies!):

static inline FunctionType appendClass(SelectorFragment& fragment, bool ((*matcher)())) {
    fragment.unoptimizedPseudoClassesWithContext.append(JSC::FunctionPtr(matcher));
    return FunctionType::SelectorCheckerWithCheckingContext;
}

Then each case will look like:

case CSSSelector::PseudoClassWindowInactive:
    return appendClass(fragment, scrollbarMatchesWindowInactivePseudoClass<CheckingContext>);

This will eliminate a lot of redundant code.

[Benjamin Poulain]

Comment on attachment 235607 [details]
Patch

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

> Source/WebCore/css/ElementRuleCollector.cpp:-296
> -            ASSERT_WITH_MESSAGE(m_pseudoStyleRequest.pseudoId == NOPSEUDO, "When matching pseudo elements, we should never compile a selector checker without context. ElementRuleCollector::collectMatchingRulesForList() should filter out useless rules for pseudo elements.");

That's very unfortunate, but that make sense :(

> Source/WebCore/cssjit/SelectorCompiler.cpp:501
> +    // These pseudo-classes only have meaning with scrollbars.
> +    case CSSSelector::PseudoClassHorizontal:
> +    case CSSSelector::PseudoClassVertical:
> +    case CSSSelector::PseudoClassDecrement:
> +    case CSSSelector::PseudoClassIncrement:
> +    case CSSSelector::PseudoClassStart:
> +    case CSSSelector::PseudoClassEnd:
> +    case CSSSelector::PseudoClassDoubleButton:
> +    case CSSSelector::PseudoClassSingleButton:
> +    case CSSSelector::PseudoClassNoButton:
> +    case CSSSelector::PseudoClassCornerPresent:
> +        return FunctionType::CannotMatchAnything;

woooot, that's a lot off the todo list :)

> Source/WebCore/cssjit/SelectorCompiler.cpp:677
> +    return type >= CSSSelector::PseudoElementScrollbar
> +        && type <= CSSSelector::PseudoElementScrollbarTrackPiece;

Could be on one line.

[Alex Christensen]

Created attachment 235700 [details]
Patch

[Benjamin Poulain]

Comment on attachment 235700 [details]
Patch

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

The patch looks good but I need to spend some time understanding what the heck are all the exceptions on in SelectorChecker before r+ing.

> Source/WebCore/css/SelectorChecker.cpp:-824
> -    // FIXME: This is a temporary hack for resizers and scrollbar corners. Eventually :window-inactive should become a real
> -    // pseudo class and just apply to everything.
> -    if (selector->pseudoClassType() == CSSSelector::PseudoClassWindowInactive)
> -        return !document->page()->focusController().isActive();

Aren't you breaking WindowInactive in the weird cases without "scrollbar"?

> LayoutTests/scrollbars/scrollbar-selectors.html:54
> +    document.head.appendChild(style);

It may be useful to force a synchronous layout here to force a style recalc for each additional rule.

> LayoutTests/scrollbars/scrollbar-selectors.html:104
> +

The text inside each block will cause the layout test to be less generic than it needs to be.

After you have run the tests above, you could have a pass removing the text of every block and replacing it by a block of fixed size. If the test does not have native widgets, nor text, it should work across all ports.

> LayoutTests/scrollbars/scrollbar-selectors.html:109
> +<body>
> +<div style="overflow: scroll">

It would be useful to add a description of what is tested here, and what should appear on screen when the test pass.

[Alex Christensen]

(In reply to comment #16)
> (From update of attachment 235700 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=235700&action=review
> 
> The patch looks good but I need to spend some time understanding what the heck are all the exceptions on in SelectorChecker before r+ing.
> 
> > Source/WebCore/css/SelectorChecker.cpp:-824
> > -    // FIXME: This is a temporary hack for resizers and scrollbar corners. Eventually :window-inactive should become a real
> > -    // pseudo class and just apply to everything.
> > -    if (selector->pseudoClassType() == CSSSelector::PseudoClassWindowInactive)
> > -        return !document->page()->focusController().isActive();
> 
> Aren't you breaking WindowInactive in the weird cases without "scrollbar"?
LayoutTests/fast/selectors/selection-window-inactive.html is not broken.
> 
> > LayoutTests/scrollbars/scrollbar-selectors.html:54
> > +    document.head.appendChild(style);
> 
> It may be useful to force a synchronous layout here to force a style recalc for each additional rule.
Done.
> 
> > LayoutTests/scrollbars/scrollbar-selectors.html:104
> > +
> 
> The text inside each block will cause the layout test to be less generic than it needs to be.
> 
> After you have run the tests above, you could have a pass removing the text of every block and replacing it by a block of fixed size. If the test does not have native widgets, nor text, it should work across all ports.
Done.
> 
> > LayoutTests/scrollbars/scrollbar-selectors.html:109
> > +<body>
> > +<div style="overflow: scroll">
> 
> It would be useful to add a description of what is tested here, and what should appear on screen when the test pass.
Done

[Alex Christensen]

Created attachment 235717 [details]
Patch

[Benjamin Poulain]

Comment on attachment 235717 [details]
Patch

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

The JIT part looks great.

Can you please also add ref-tests for Resizer+window inactive?

We should talk about mixing those pseudo elements with other elements when you have 5 minutes. We may want to improve the RuleCollector's partitioning.

> Source/WebCore/css/ElementRuleCollector.cpp:-296
> -            ASSERT_WITH_MESSAGE(m_pseudoStyleRequest.pseudoId == NOPSEUDO, "When matching pseudo elements, we should never compile a selector checker without context. ElementRuleCollector::collectMatchingRulesForList() should filter out useless rules for pseudo elements.");

Ok, I know how to fix this:

We could assert than if the selector checker match, we must have that m_pseudoStyleRequest.pseudoId == NOPSEUDO.

> Source/WebCore/css/SelectorChecker.cpp:481
>          } else if (context.hasScrollbarPseudo) {

This can be executed for RESIZER. In that case, context.scrollbar could be null, and you would break PseudoClassWindowInactive.
Can you please also add a test for that?

> Source/WebCore/css/SelectorCheckerTestFunctions.h:167
> +template <class ContextType>

For genericity, we generally use "typename" instead of "class" for templates.

We could remove "element" and "scope" from the context so that it works the same way in both implementations.

> Source/WebCore/cssjit/SelectorCompiler.cpp:687
> +        if (relation == CSSSelector::SubSelector
> +            && fragment.pseudoElementSelector
> +            && !isScrollbarPseudoElement(fragment.pseudoElementSelector->pseudoElementType())) {
> +            return FunctionType::CannotMatchAnything;
> +        }

This is straightforward and it looks correct.

What we have in SelectorChecker is a mess and it is hard to tell if it is correct (I actually somewhat doubt it is). Can you please rewrite SelectorChecker's SubSelector handling to have the exact same logic? (you can make SelectorChecker as inefficient as you want to make it equivalent, at this point it really does not matter).

[Alex Christensen]

Created attachment 235833 [details]
Patch

[Alex Christensen]

You were right.  window-inactive was broken with resizers and corners.  I kept the same logic, but I now access the document through the contexts, and I added tests.

[Alex Christensen]

Created attachment 236144 [details]
Patch

[Benjamin Poulain]

Comment on attachment 236144 [details]
Patch

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

LGTM! Let's do it!

> Source/WebCore/css/SelectorChecker.cpp:163
> +        ASSERT(context.scrollbar);

You could return true here after the assertion.

> Source/WebCore/css/SelectorChecker.cpp:167
> +    return dynamicPseudo != NOPSEUDO && (context.scrollbar || dynamicPseudo == RESIZER);

Then replace this line by "return dynamicPseudo == RESIZER;".

> Source/WebCore/cssjit/SelectorCompiler.cpp:371
> +        fragment.unoptimizedPseudoClasses.append(JSC::FunctionPtr(isWindowInactive));

ooooh. Yep, that's much simpler :)

[Alex Christensen]

http://trac.webkit.org/changeset/172220

[Yusuke Suzuki]

Great!