Bug 59228 – Remove IsShadowRootFlag from Node

• Status: RESOLVED
• Resolution: WONTFIX
• Component: DOM

[Dimitri Glazkov (Google)]

Once shadow DOM always starts with the ShadowRoot element, we no longer need this flag.

[Dimitri Glazkov (Google)]

Created attachment 96302 [details]
Patch

[Darin Adler]

Comment on attachment 96302 [details]
Patch

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

> Source/WebCore/dom/Node.h:213
> -    bool isShadowRoot() const { return getFlag(IsShadowRootFlag); }
> -    // FIXME: Remove this when all shadow roots are ShadowRoots.
> -    virtual bool isShadowBoundary() const { return false; }
> +    virtual bool isShadowRoot() const { return false; }

A virtual function is not as fast as checking a bit. And in the past we have optimized various code paths by replacing virtual functions with a bit in Node.

Are you sure that this does not create a slowdown?

[Dimitri Glazkov (Google)]

(In reply to comment #2)
> (From update of attachment 96302 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=96302&action=review
> 
> > Source/WebCore/dom/Node.h:213
> > -    bool isShadowRoot() const { return getFlag(IsShadowRootFlag); }
> > -    // FIXME: Remove this when all shadow roots are ShadowRoots.
> > -    virtual bool isShadowBoundary() const { return false; }
> > +    virtual bool isShadowRoot() const { return false; }
> 
> A virtual function is not as fast as checking a bit. And in the past we have optimized various code paths by replacing virtual functions with a bit in Node.
> 
> Are you sure that this does not create a slowdown?

I will check. I wrote a microbench for this a while back.

[Dimitri Glazkov (Google)]

Comment on attachment 96302 [details]
Patch

Actually, this patch sucks. I can do better.

[Dimitri Glazkov (Google)]

This patch has at least two parts. I need to split out the simple ones.

[Dominic Cooney]

Actually we tried replacing this with a virtual, but it regressed performance because parentNode needs to test isShadowRoot; see bug 69266. For now I think we assume that the node flag stays.