http://trac.webkit.org/changeset/84225 from bug 52788 added a virtual function call (isSVGShadowRoot()) to Node::parentNode():
inline ContainerNode* Node::parentNode() const
- return getFlag(IsShadowRootFlag) ? 0 : parent();
+ return getFlag(IsShadowRootFlag) || isSVGShadowRoot() ? 0 : parent();
This regresses everything that depends on walking up the tree, most crucially CSS selector matching.
The fact that the call is carefully inlined should be a hint that you are not supposed to put a virtual call here.
OK. Does SVGShadowRoot warrant its own flag to avoid this?
Why does it need a separate test? It makes no logical sense that a "SVG shadow root" is not a "shadow root".
We are running out of bits in Node so preferably not.
Originally there was an attempt to merge ShadowRoot and SVGShadowRoot, but it turned out to be difficult—dglazkov should be able to provide context. I attempted to separate them which is what motivated this change.
To add some commentary on what was difficult about merging ShadowRoot and SVGShadowRoot—IIRC:
- Rendering is different.
- SVGShadowRoot tries to behave like <svg:g>, which is not helpful when ShadowRoot appears in a HTML tree.
- Event dispatch is different.
- There’s a handful of special behavior for SVGShadowRoot, eg can’t have <script> descendants.
- There’s a handful of special behavior for ShadowRoot around editing.
CC’d Krit in case he is working on any refactoring around SVGShadowRoot.
I think that sums it up pretty nicely. One other approach we were discussing is to set the parent pointer to NULL, the host node holding an explicit ref on the (SVG) shadow root instead, and having a separate host element pointer on the (SVG) shadow root.
However, until we add ShadowRoot to SVG, this merely pushes the virtual function call from parentNode() to parentOrHostNode().
Maybe right now we can move back to using the 'shadow root' flag for ShadowRoot and SVGShadowRoot. This will make parentNode() fast. Some editing code may slow down, although maybe we can make isShadowRoot do
getFlag(ShadowRoot) && !n->isSVGShadowRoot()
which is still fast in <input>, <textarea>, etc. when the result is false.
Another awkward difference between ShadowRoot and SVGShadowRoot is ShadowRoot sets and clears the flag when it goes into and out of the tree, but isSVGShadowRoot is constant.
A big hacky, but parentNode can be trivially sped up in the non-SVG case with something like:
return getFlag(IsShadowRootFlag) || (isSVGElement() && isSVGShadowRoot()) ? 0 : parent();
which predicates the virtual call on another flag test.
For now I am going to look at using these pairs of flags:
IsShadowRootFlag, not IsSVGElementFlag => ShadowRoot
IsShadowRootFlag, IsSVGElementFlag => SVGShadowRoot
I will rename the schizoflag to IsShadowRootOrSVGShadowRootFlag.
Created attachment 109801 [details]
Depends on patch on 69406.
(In reply to comment #10)
Looks good! :)
Comment on attachment 109801 [details]
Clearing flags on attachment: 109801
Committed r96803: <http://trac.webkit.org/changeset/96803>
All reviewed patches have been landed. Closing bug.