Create a skeleton for CSS Selector code generation
Created attachment 219722 [details] Patch
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.
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
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.
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.
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.
Committed r160983: <http://trac.webkit.org/changeset/160983>
Landed :) I left the gotos in because they will go away later when we handle more cases. Thanks for the reviews!