Bug 59228 - Remove IsShadowRootFlag from Node
Summary: Remove IsShadowRootFlag from Node
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Dimitri Glazkov (Google)
URL:
Keywords:
Depends on: 48698
Blocks: 72352 59802
  Show dependency treegraph
 
Reported: 2011-04-22 14:06 PDT by Dimitri Glazkov (Google)
Modified: 2011-11-20 17:58 PST (History)
3 users (show)

See Also:


Attachments
Patch (17.51 KB, patch)
2011-06-07 14:55 PDT, Dimitri Glazkov (Google)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.