Bug 59228

Summary: Remove IsShadowRootFlag from Node
Product: WebKit Reporter: Dimitri Glazkov (Google) <dglazkov>
Component: DOMAssignee: Dimitri Glazkov (Google) <dglazkov>
Status: RESOLVED WONTFIX    
Severity: Normal CC: dominicc, morrita, rolandsteiner
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on: 48698    
Bug Blocks: 72352, 59802    
Attachments:
Description Flags
Patch none

Description Dimitri Glazkov (Google) 2011-04-22 14:06:38 PDT
Once shadow DOM always starts with the ShadowRoot element, we no longer need this flag.
Comment 1 Dimitri Glazkov (Google) 2011-06-07 14:55:22 PDT
Created attachment 96302 [details]
Patch
Comment 2 Darin Adler 2011-06-07 16:24:11 PDT
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?
Comment 3 Dimitri Glazkov (Google) 2011-06-07 17:13:59 PDT
(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.
Comment 4 Dimitri Glazkov (Google) 2011-06-07 17:14:22 PDT
Comment on attachment 96302 [details]
Patch

Actually, this patch sucks. I can do better.
Comment 5 Dimitri Glazkov (Google) 2011-06-08 12:46:42 PDT
This patch has at least two parts. I need to split out the simple ones.
Comment 6 Dominic Cooney 2011-11-20 17:58:35 PST
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.