Bug 126044 – Create a skeleton for CSS Selector code generation

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Benjamin Poulain]

Create a skeleton for CSS Selector code generation

[Benjamin Poulain]

Created attachment 219722 [details]
Patch

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/assembler/LinkBuffer.h', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/cssjit/CSSSelectorCompiler.cpp', u'Source/WebCore/cssjit/CSSSelectorCompiler.h', u'Source/WebCore/dom/Element.cpp', u'Source/WebCore/dom/Element.h', u'Source/WebCore/dom/Node.h', u'Source/WebCore/dom/QualifiedName.h', '--commit-queue']" exit_code: 1
ERROR: Source/JavaScriptCore/assembler/LinkBuffer.h:323:  Wrong number of spaces before statement. (expected: 9)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/LinkBuffer.h:326:  Wrong number of spaces before statement. (expected: 9)  [whitespace/indent] [4]
Total errors found: 2 in 10 files


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

[EFL EWS Bot]

Comment on attachment 219722 [details]
Patch

Attachment 219722 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/50208169

[Gavin Barraclough]

Comment on attachment 219722 [details]
Patch

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

Looks good, but I haven't reviewed the css logic well enough to r+, would probably be good if someone else looked over this too.  If you don't get any takers I'll give it a second pass.

> Source/WebCore/cssjit/CSSSelectorCompiler.cpp:659
> +    m_assembler.jump().linkTo(m_indirectAdjacentEntryPoint, &m_assembler);

->
    m_assembler.jump(m_indirectAdjacentEntryPoint);

> Source/WebCore/cssjit/CSSSelectorCompiler.cpp:668
> +    m_assembler.jump().linkTo(m_descendantEntryPoint, &m_assembler);

->
    m_assembler.jump(m_descendantEntryPoint);

> Source/WebCore/cssjit/CSSSelectorCompiler.cpp:731
> +        failureCases.append(m_assembler.branchPtr(Assembler::NotEqual, Assembler::Address(qualifiedNameImpl, QualifiedName::QualifiedNameImpl::namespaceMemoryOffset()), constantRegister));

I think you can do this in one line (get rid of the LocalRegister & move) using:
    Jump branchPtr(RelationalCondition cond, Address left, TrustedImmPtr right)

> Source/WebCore/cssjit/CSSSelectorCompiler.cpp:739
> +        failureCases.append(m_assembler.branchPtr(Assembler::NotEqual, Assembler::Address(qualifiedNameImpl, QualifiedName::QualifiedNameImpl::localNameMemoryOffset()), constantRegister));

ditto

> Source/WebCore/dom/Node.h:179
> +#endif // ENABLE(CSS_SELECTOR_JIT)

Do we really need the compile guards for all these accessors? – I'd remove them.
If you keep them, might be nice to merge these two #if/#endif blocks.

[Benjamin Poulain]

Thanks for the review!

(In reply to comment #4)
> > Source/WebCore/cssjit/CSSSelectorCompiler.cpp:731
> > +        failureCases.append(m_assembler.branchPtr(Assembler::NotEqual, Assembler::Address(qualifiedNameImpl, QualifiedName::QualifiedNameImpl::namespaceMemoryOffset()), constantRegister));
> 
> I think you can do this in one line (get rid of the LocalRegister & move) using:
>     Jump branchPtr(RelationalCondition cond, Address left, TrustedImmPtr right)

This one was intentional.
    branchPtr(RelationalCondition cond, Address left, TrustedImmPtr right)
uses the scratch register of the MacroAssembler. Here all registers are handled through the register allocator.

[Antti Koivisto]

Comment on attachment 219722 [details]
Patch

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

Looks great!

> Source/WebCore/cssjit/CSSSelectorCompiler.cpp:200
> +    SelectorFragment fragment;

I like SelectorFragment. Maybe we should use the fragment terminology everywhere.

> Source/WebCore/cssjit/CSSSelectorCompiler.cpp:210
> +        switch (selector->m_match) {
> +        case CSSSelector::Tag:
> +            ASSERT(!fragment.tagName);
> +            fragment.tagName = &(selector->tagQName());
> +            break;
> +        default:
> +            goto CannotHandleSelector;
> +        }

It would be better to list all match types instead of having a default.

> Source/WebCore/cssjit/CSSSelectorCompiler.cpp:235
> +    return;
> +
> +CannotHandleSelector:
> +    m_selectorFragments.clear();

I suspect this would read better if you factored the fragment initialisation to a function. You could avoid clumsy gotos.

> Source/WebCore/cssjit/CSSSelectorCompiler.cpp:693
> +        m_stackAllocator.merge(std::move(successStack), std::move(adjacentTailStack), std::move(descendantTailStack));
> +    } else if (fragment.backtrackingFlags & BacktrackingFlag::DirectAdjacentTail) {

You could do early returns instead of else.

> Source/WebCore/cssjit/CSSSelectorCompiler.h:67
> +namespace CSSSelectorCompiler {

I would prefer SelectorCompiler. It is not ambiguous and we try to use CSS* prefix to refer to CSSOM types only. Same with CSSSelectorCompilationStatus.

> Source/WebCore/dom/Element.h:377
> +    static void setChildrenAffectedByDirectAdjacentRules(Element*);
>      void setChildrenAffectedByDirectAdjacentRules() { setFlag(ChildrenAffectedByDirectAdjacentRulesFlag); }
> -    void setChildrenAffectedByForwardPositionalRules();
> +    static void setChildrenAffectedByForwardPositionalRules(Element*);
> +    void setChildrenAffectedByForwardPositionalRules() { setChildrenAffectedByForwardPositionalRules(this); }

Should really eliminate the remaining side effects at some point.

[Benjamin Poulain]

Committed r160983: <http://trac.webkit.org/changeset/160983>

[Benjamin Poulain]

Landed :)
I left the gotos in because they will go away later when we handle more cases.

Thanks for the reviews!