Bug 166748

Created attachment 298151 [details] Reduction

<rdar://problem/29649177>

*** Bug 184273 has been marked as a duplicate of this bug. ***

Created attachment 349116 [details] WIP

Comment on attachment 349116 [details] WIP Attachment 349116 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/9124393 New failing tests: fast/selectors/nth-last-child-cannot-match-during-parsing-1.html fast/selectors/nth-last-child-of-cannot-match-during-parsing-1.html

Created attachment 349124 [details] Archive of layout-test-results from ews103 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-sierra Platform: Mac OS X 10.12.6

Comment on attachment 349116 [details] WIP Attachment 349116 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/9124403 New failing tests: fast/selectors/nth-last-child-cannot-match-during-parsing-1.html fast/selectors/nth-last-child-of-cannot-match-during-parsing-1.html

Created attachment 349125 [details] Archive of layout-test-results from ews107 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6

Comment on attachment 349116 [details] WIP Attachment 349116 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/9124456 New failing tests: fast/selectors/nth-last-child-cannot-match-during-parsing-1.html fast/selectors/nth-last-child-of-cannot-match-during-parsing-1.html

Created attachment 349127 [details] Archive of layout-test-results from ews112 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews112 Port: mac-sierra Platform: Mac OS X 10.12.6

Comment on attachment 349116 [details] WIP Attachment 349116 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/9124513 New failing tests: fast/selectors/nth-last-child-cannot-match-during-parsing-1.html fast/selectors/nth-last-child-of-cannot-match-during-parsing-1.html

Created attachment 349128 [details] Archive of layout-test-results from ews125 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.4

Created attachment 349252 [details] Fixes the bug

Comment on attachment 349252 [details] Fixes the bug View in context: https://bugs.webkit.org/attachment.cgi?id=349252&action=review The approach looks good to me. I have some suggestions to clean up CSS JIT codes. > Source/WebCore/ChangeLog:3 > + :first-child, :last-child, :nth-child, and :nth-of-type don't work on shadow rootâs children Let's use ascii `'` for `root's`. > Source/WebCore/css/SelectorChecker.cpp:747 > + if (!isFirstChild) Why not performing isFirstChild here? if (!isFirstChildElement(element)) break; Ah, I see. The implementation is aligned to CSS JIT one. Make sense. > Source/WebCore/css/SelectorChecker.cpp:768 > + if (!parentElement.isFinishedParsingChildren()) It is not checked in the case of shadow root, but it is OK since shadow root children are not populated by parsing, is my understanding correct? > Source/WebCore/cssjit/SelectorCompiler.cpp:3200 > + Assembler::RegisterID parentNode = m_registerAllocator.allocateRegister(); Let's use `LocalRegister parentNode(m_registerAllocator)`. > Source/WebCore/cssjit/SelectorCompiler.cpp:3225 > + m_assembler.jump().linkTo(checkWithoutRelation, &m_assembler); Remove this jump, fallthough to checkWithoutRelation. > Source/WebCore/cssjit/SelectorCompiler.cpp:3227 > + m_registerAllocator.deallocateRegister(parentNode); Remove this by using LocalRegister. > Source/WebCore/cssjit/SelectorCompiler.cpp:3228 > + How about putting checkWithoutRelation here? Assembler::Label checkWithoutRelation(m_assembler.label()); notResolvingStyle.link(&m_assembler); failureCases.append(m_assembler.branchTest32(Assembler::NonZero, isFirstChildRegister)); // fallthough to successCases. > Source/WebCore/cssjit/SelectorCompiler.cpp:3294 > + Assembler::RegisterID parentNode = m_registerAllocator.allocateRegister(); Let's use `LocalRegister parentNode(m_registerAllocator)`. > Source/WebCore/cssjit/SelectorCompiler.cpp:3335 > + m_assembler.jump().linkTo(checkWithoutRelation, &m_assembler); Remove this jump, fallthough to checkWithoutRelation. > Source/WebCore/cssjit/SelectorCompiler.cpp:3337 > + m_registerAllocator.deallocateRegister(parentNode); Remove this by using LocalRegister. > Source/WebCore/cssjit/SelectorCompiler.cpp:3338 > + How about putting checkWithoutRelation here? Assembler::Label checkWithoutRelation(m_assembler.label()); notResolvingStyle.link(&m_assembler); failureCases.append(m_assembler.branchTest32(Assembler::NonZero, isLastChildRegister)); // fallthough to successCases. > Source/WebCore/cssjit/SelectorCompiler.cpp:3362 > + Assembler::RegisterID parentNode = m_registerAllocator.allocateRegister(); Ditto. > Source/WebCore/cssjit/SelectorCompiler.cpp:3405 > + Assembler::Label checkWithoutRelation(m_assembler.label()); > notResolvingStyle.link(&m_assembler); > failureCases.append(m_assembler.branchTest32(Assembler::NonZero, isOnlyChildRegister)); > + successCases.append(m_assembler.jump()); Ditto. > Source/WebCore/cssjit/SelectorCompiler.cpp:3412 > + m_registerAllocator.deallocateRegister(parentNode); Ditto. > Source/WebCore/cssjit/SelectorCompiler.cpp:3557 > +void SelectorCodeGenerator::generateNthChildParentCheckAndRelationUpdate(Assembler::JumpList& failureCases, const SelectorFragment& fragment) Nice cleanup. > Source/WebCore/cssjit/SelectorCompiler.cpp:3679 > +void SelectorCodeGenerator::generateNthLastChildParentCheckAndRelationUpdate(Assembler::JumpList& failureCases, const SelectorFragment& fragment) Nice cleanup. > Source/WebCore/domjit/DOMJITHelpers.h:192 > +inline CCallHelpers::Jump branchTestIsShadowRootFlagOnNode(MacroAssembler& jit, CCallHelpers::ResultCondition condition, GPRReg nodeAddress) > +{ > + return jit.branchTest32(condition, CCallHelpers::Address(nodeAddress, Node::nodeFlagsMemoryOffset()), CCallHelpers::TrustedImm32(Node::flagIsShadowRoot())); > +} > + > +inline CCallHelpers::Jump branchTestIsElementOrShadowRootFlagOnNode(MacroAssembler& jit, CCallHelpers::ResultCondition condition, GPRReg nodeAddress) > +{ > + return jit.branchTest32(condition, CCallHelpers::Address(nodeAddress, Node::nodeFlagsMemoryOffset()), > + CCallHelpers::TrustedImm32(Node::flagIsShadowRoot() | Node::flagIsElement())); > +} > + Nice.

Comment on attachment 349252 [details] Fixes the bug View in context: https://bugs.webkit.org/attachment.cgi?id=349252&action=review > Source/WebCore/css/SelectorChecker.cpp:748 > + break; Yeah, and also with isLastChild case where we'd have to set the flag to false when parentElement.isFinishedParsingChildren() is false. >> Source/WebCore/css/SelectorChecker.cpp:768 >> + if (!parentElement.isFinishedParsingChildren()) > > It is not checked in the case of shadow root, but it is OK since shadow root children are not populated by parsing, is my understanding correct? Yes, the flag is only available on an element. The HTML parser never appends a child to a ShadowRoot. >> Source/WebCore/cssjit/SelectorCompiler.cpp:3200 >> + Assembler::RegisterID parentNode = m_registerAllocator.allocateRegister(); > > Let's use `LocalRegister parentNode(m_registerAllocator)`. Oh yeah, I was wondering about that. Fixed. >> Source/WebCore/cssjit/SelectorCompiler.cpp:3225 >> + m_assembler.jump().linkTo(checkWithoutRelation, &m_assembler); > > Remove this jump, fallthough to checkWithoutRelation. Fixed. >> Source/WebCore/cssjit/SelectorCompiler.cpp:3227 >> + m_registerAllocator.deallocateRegister(parentNode); > > Remove this by using LocalRegister. Yup. >> Source/WebCore/cssjit/SelectorCompiler.cpp:3228 >> + > > How about putting checkWithoutRelation here? > > Assembler::Label checkWithoutRelation(m_assembler.label()); > notResolvingStyle.link(&m_assembler); > failureCases.append(m_assembler.branchTest32(Assembler::NonZero, isFirstChildRegister)); > // fallthough to successCases. Oh, that's a lot cleaner! Fixed here and for generateElementIsLastChild. >> Source/WebCore/cssjit/SelectorCompiler.cpp:3294 >> + Assembler::RegisterID parentNode = m_registerAllocator.allocateRegister(); > > Let's use `LocalRegister parentNode(m_registerAllocator)`. Fixed. >> Source/WebCore/cssjit/SelectorCompiler.cpp:3335 >> + m_assembler.jump().linkTo(checkWithoutRelation, &m_assembler); > > Remove this jump, fallthough to checkWithoutRelation. Yup, applied the same refactoring here. >> Source/WebCore/cssjit/SelectorCompiler.cpp:3337 >> + m_registerAllocator.deallocateRegister(parentNode); > > Remove this by using LocalRegister. Fixed. >> Source/WebCore/cssjit/SelectorCompiler.cpp:3338 >> + > > How about putting checkWithoutRelation here? > > Assembler::Label checkWithoutRelation(m_assembler.label()); > notResolvingStyle.link(&m_assembler); > failureCases.append(m_assembler.branchTest32(Assembler::NonZero, isLastChildRegister)); > // fallthough to successCases. Fixed. >> Source/WebCore/cssjit/SelectorCompiler.cpp:3362 >> + Assembler::RegisterID parentNode = m_registerAllocator.allocateRegister(); > > Ditto. Fixed. >> Source/WebCore/cssjit/SelectorCompiler.cpp:3405 >> + successCases.append(m_assembler.jump()); > > Ditto. Fixed. >> Source/WebCore/cssjit/SelectorCompiler.cpp:3412 >> + m_registerAllocator.deallocateRegister(parentNode); > > Ditto. Fixed.

Created attachment 349359 [details] Addressed the review comments

Comment on attachment 349359 [details] Addressed the review comments r=me

Committed r235917: <https://trac.webkit.org/changeset/235917>