Make GetFlattenedTreeParent more straight-forward.

RESOLVED FIXED in Firefox 59

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: emilio, Assigned: emilio)

Tracking

unspecified
mozilla59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox59 fixed)

Details

Attachments

(1 attachment)

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.
Attachment #8939278 - Flags: review?(bugs) → review-
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.
Attachment #8939278 - Flags: review?(bugs) → review+
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b8656e137157
Make GetFlattenedTreeParent more straight-forward. r=smaug
https://hg.mozilla.org/mozilla-central/rev/b8656e137157
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.