[webkit-reviews] review granted: [Bug 126185] Use the Selector Code Generator for matching in SelectorQuery : [Attachment 219936] Patch

Mon Dec 23 16:07:28 PST 2013

Ryosuke Niwa <rniwa at webkit.org> has granted Benjamin Poulain
<benjamin at webkit.org>'s request for review:
Bug 126185: Use the Selector Code Generator for matching in SelectorQuery
https://bugs.webkit.org/show_bug.cgi?id=126185

Attachment 219936: Patch
https://bugs.webkit.org/attachment.cgi?id=219936&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=219936&action=review

> Source/WebCore/dom/SelectorQuery.cpp:329
> +#if ENABLE(CSS_SELECTOR_JIT)
> +	       void* compiledSelectorChecker =
selectorData.compiledSelectorCodeRef.code().executableAddress();
> +	       if (!compiledSelectorChecker && selectorData.compilationStatus
== SelectorCompilationStatus::NotCompiled) {
> +		   JSC::VM* vm =
rootNode.document().scriptExecutionContext()->vm();
> +		   selectorData.compilationStatus =
SelectorCompiler::compileSelector(selectorData.selector, vm,
selectorData.compiledSelectorCodeRef);
> +	       }
> +
> +	       if (compiledSelectorChecker) {
> +		   if (selectorData.compilationStatus ==
SelectorCompilationStatus::SimpleSelectorChecker) {
> +		       SelectorCompiler::SimpleSelectorChecker selectorChecker
= SelectorCompiler::simpleSelectorCheckerFunction(compiledSelectorChecker,
selectorData.compilationStatus);
> +		      
executeCompiledSimpleSelectorChecker<SelectorQueryTrait>(rootNode,
selectorChecker, output);
> +		   } else {
> +		       ASSERT(selectorData.compilationStatus ==
SelectorCompilationStatus::SelectorCheckerWithCheckingContext);
> +		       SelectorCompiler::SelectorCheckerWithCheckingContext
selectorChecker =
SelectorCompiler::selectorCheckerFunctionWithCheckingContext(compiledSelectorCh
ecker, selectorData.compilationStatus);
> +
> +		       SelectorCompiler::CheckingContext context;
> +		       context.elementStyle = nullptr;
> +		       context.resolvingMode = SelectorChecker::QueryingRules;
> +		      
executeCompiledSelectorCheckerWithContext<SelectorQueryTrait>(rootNode,
selectorChecker, context, output);
> +		   }
> +		   return;
> +	       }
> +#endif // ENABLE(CSS_SELECTOR_JIT)

This function is getting really big now. Would it make sense to extract this as
a separate inline function instead?

> Source/WebCore/dom/SelectorQuery.h:63
> +	   mutable SelectorCompilationStatus compilationStatus;
> +	   mutable JSC::MacroAssemblerCodeRef compiledSelectorCodeRef;

It seems like we could encapsulate these two variables into one struct in
SelectorCompiler.h

> LayoutTests/fast/selectors/querySelector-long-adjacent-backtracking.html:4
> +<script src="../../resources/js-test-pre.js"></script>

We can put this in the body.

> LayoutTests/fast/selectors/querySelector-long-child-backtracking.html:4
> +<script src="../../resources/js-test-pre.js"></script>

Ditto.

>
LayoutTests/fast/selectors/querySelector-mixed-child-adjacent-backtracking.html
:4
> +<script src="../../resources/js-test-pre.js"></script>

Ditto.

>
LayoutTests/fast/selectors/querySelector-multiple-simple-child-backtracking.htm
l:4
> +<script src="../../resources/js-test-pre.js"></script>

Ditto.

> LayoutTests/fast/selectors/querySelector-simple-adjacent-backtracking.html:3
> +<head>

Ditto.

> LayoutTests/fast/selectors/querySelector-simple-child-backtracking.html:4
> +<script src="../../resources/js-test-pre.js"></script>

Ditto.