Now that a bunch of the stuff we need to implement it isn't virtual (bug 1427001), we can speed it up a fair bit in the rare case while keeping the normal case fast.

Comment on attachment 8939278 [details] Bug 1427511: Make GetFlattenedTreeParent more straight-forward. https://reviewboard.mozilla.org/r/209710/#review215254 Could I get some clarifications and I think IsShadowRoot() should live in nsINode ::: dom/base/FragmentOrElement.cpp:196 (Diff revision 2) > - > - return parentSlot; > -} > > -nsINode* > -nsIContent::GetFlattenedTreeParentNodeInternal(FlattenedParentType aType) const > + nsIContent* parent = GetParent(); > + if (parent != OwnerDoc()->GetRootElement()) { So MOZ_ASSERT(!GetParent()->GetParent()) but then we still may have parent which isn't root element. Could you explain this a bit more? Add some comment. ::: dom/base/nsIContent.h:191 (Diff revision 2) > { > return HasFlag(NODE_IS_NATIVE_ANONYMOUS_ROOT | > NODE_IS_ROOT_OF_CHROME_ONLY_ACCESS); > } > > + bool IsShadowRoot() const Why is this in nsIContent? Looks like the code would be a tad simpler if this lived in nsINode ::: dom/base/nsIContent.h:621 (Diff revision 2) > - inline nsIContent *GetFlattenedTreeParent() const; > + inline nsIContent* GetFlattenedTreeParent() const; > > - // Helper method, which we leave public so that it's accessible from nsINode. > - enum FlattenedParentType { eNotForStyle, eForStyle }; > - nsINode* GetFlattenedTreeParentNodeInternal(FlattenedParentType aType) const; > + /** > + * Get the flattened tree parent for NAC holding from the root element. > + */ > + nsINode* GetFlattenedTreeParentForRootElementNAC() const; I have real trouble to understand what the method returns. Root element of what? Is this about the document element, top most element? ::: dom/base/nsNodeUtils.cpp:85 (Diff revision 2) > NS_OBSERVER_AUTO_ARRAY_NOTIFY_OBSERVERS( \ > slots->mMutationObservers, nsIMutationObserver, 1, \ > func_, params_); \ > } \ > last = node; \ > - if (ShadowRoot* shadow = ShadowRoot::FromNode(node)) { \ > + if (node->IsContent() && node->AsContent()->IsShadowRoot()) { \ Doesn't this make the code a tad slower.

Comment on attachment 8939278 [details] Bug 1427511: Make GetFlattenedTreeParent more straight-forward. https://reviewboard.mozilla.org/r/209710/#review215254 > Why is this in nsIContent? Looks like the code would be a tad simpler if this lived in nsINode Sure > I have real trouble to understand what the method returns. > Root element of what? > Is this about the document element, top most element? It should be the root element, will add some comments. > Doesn't this make the code a tad slower. How? `IsContent()` is a bit check, and `IsShadowRoot` is cheap. Before this it used to be a function call that contained more. But yeah I guess the `IsContent` check is unnecessary if it lives in `nsINode`.

yeah, I was thinking that IsContent() check.

Comment on attachment 8939278 [details] Bug 1427511: Make GetFlattenedTreeParent more straight-forward. https://reviewboard.mozilla.org/r/209710/#review215254 > So MOZ_ASSERT(!GetParent()->GetParent()) but > then we still may have parent which isn't root element. > Could you explain this a bit more? Add some comment. You can technically be in a detached subtree where the parent doesn't have a parent. Checking `!GetParent()->GetParent()` seemed faster than checking `OwnerDoc()->GetRootElement() == GetParent()`. But it's not much of a difference I guess.

Comment on attachment 8939278 [details] Bug 1427511: Make GetFlattenedTreeParent more straight-forward. https://reviewboard.mozilla.org/r/209710/#review215290 ::: dom/base/nsIContent.h:618 (Diff revision 3) > + * > + * Document-level anonymous content holds from the root element, even though > + * they should not be treated as such (they should be parented to the document > + * instead, and shouldn't inherit from the document element). > + */ > + nsINode* GetFlattenedTreeParentForRootElementNAC() const; I know it is longer, but could you call this ...DocumentElementNAC, since that would map to the spec terminology. Root of an nsIContent (Element for example) can be something else too than document. And this method seems to deal with the case only when root is document. ::: dom/base/nsIContentInlines.h:67 (Diff revision 3) > -template<nsIContent::FlattenedParentType Type> > -static inline bool FlattenedTreeParentIsParent(const nsINode* aNode) > +template<bool aForStyle> > +static inline nsINode* > +GetFlattenedTreeParentNode(const nsINode* aNode) > { > - // Try to short-circuit past the complicated and not-exactly-fast logic for > - // computing the flattened parent. > + nsINode* parent = aNode->GetParentNode(); > + if (!aNode->IsContent()) { What is this case trying to capture? When is aNode non-nsIContent, yet it has a parent node? ::: dom/base/nsIContentInlines.h:90 (Diff revision 3) > > - // Common case. > - return true; > + if (content->IsRootOfAnonymousSubtree()) { > + return parent; > -} > + } > > -template<nsIContent::FlattenedParentType Type> > + if (parentAsContent->GetShadowRoot()) { This could use a comment that if parent has shadow root, yet this isn't assigned to any slot, we return null. ::: dom/base/nsIContentInlines.h:96 (Diff revision 3) > -static inline nsINode* > -GetFlattenedTreeParentNode(const nsINode* aNode) > -{ > - if (MOZ_LIKELY(FlattenedTreeParentIsParent<Type>(aNode))) { > - return aNode->GetParentNode(); > + return content->GetAssignedSlot(); > + } > + > + if (parentAsContent->IsInShadowTree()) { > + if (auto* slot = mozilla::dom::HTMLSlotElement::FromContent(parentAsContent)) { > + return slot->AssignedNodes().IsEmpty() And this could use a comment too ::: dom/base/nsIContentInlines.h:108 (Diff revision 3) > + } > + } > + > + if (content->HasFlag(NODE_MAY_BE_IN_BINDING_MNGR) || > + parent->HasFlag(NODE_MAY_BE_IN_BINDING_MNGR)) { > + if (auto* xblInsertionPoint = content->GetXBLInsertionPoint()) { Don't use auto here. It isn't clear what the type is. ::: dom/base/nsIContentInlines.h:129 (Diff revision 3) > } > > inline nsINode* > nsINode::GetFlattenedTreeParentNode() const > { > - return ::GetFlattenedTreeParentNode<nsIContent::eNotForStyle>(this); > + return ::GetFlattenedTreeParentNode<false>(this); It is unclear to me why you make the template take bool and not the existing easier to understand enum. Please explain or keep using the enum. ::: dom/base/nsNodeUtils.cpp:121 (Diff revision 3) > if (slots && !slots->mMutationObservers.IsEmpty()) { \ > NS_OBSERVER_AUTO_ARRAY_NOTIFY_OBSERVERS_WITH_QI( \ > slots->mMutationObservers, nsIMutationObserver, 1, \ > nsIAnimationObserver, func_, params_); \ > } \ > - ShadowRoot* shadow = ShadowRoot::FromNode(node); \ > + if (ShadowRoot* shadow = ShadowRoot::FromNode(node)) { \ Unrelated change, but fine.

Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/autoland/rev/b8656e137157 Make GetFlattenedTreeParent more straight-forward. r=smaug