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?
Created attachment 235436 [details] Patch
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
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
Created attachment 235446 [details] Patch
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 :(
(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
Created attachment 235554 [details] Patch
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.
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
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
Created attachment 235607 [details] Patch
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.
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.
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.
Created attachment 235700 [details] Patch
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.
(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
Created attachment 235717 [details] Patch
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).
Created attachment 235833 [details] Patch
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.
Created attachment 236144 [details] Patch
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 :)
http://trac.webkit.org/changeset/172220
Great!