Optimize Node::isInShadowTree to execute in constant-time
Created attachment 106525 [details] Patch
Comment on attachment 106525 [details] Patch Awesome.
I should ask: do y'all know of a particular test that might exercise isInShadowTree()? There are only a few callers...
(In reply to comment #3) > I should ask: do y'all know of a particular test that might exercise isInShadowTree()? There are only a few callers... Let's just write one.
(In reply to comment #4) > (In reply to comment #3) > > I should ask: do y'all know of a particular test that might exercise isInShadowTree()? There are only a few callers... > > Let's just write one. That'll require finding an entry point I understand :). The clearest usage is in HTMLLinkElement; it looks like that call was added for http://trac.webkit.org/changeset/77834, and I've run the test that patch was meant to fix (svg/dom/use-transform.svg). It passes without crashing. The other two usages are in editing/range code and in SVGTrefElement, neither of which I'm familiar enough with to exercise. Let me know if you've got something in mind...
Unless someone changed SVG while I wasn't looking, this won't work for SVG, because SVG doesn't use TreeScope yet. Indeed, SVG is the only reason why we still have various functions climbing the tree to look for, e.g., the host element (Node::shadowTreeRootNode et al) rather than doing O(1) access.
You could still fast-path the in new-style shadow case as a prefix. Maybe if you’re willing to assume SVG elements don’t display non-SVG children (pretty sure I have seen that elsewhere in SVG shadow walking code) you could test !isSVGElement and predicate the fast path on that.
This could be a bug in existing code, but it already ignores SVG shadow roots. So I think this change doesn't make things worse. Right?
(In reply to comment #8) IIRC isShadowRoot() works for both new-style shadows and SVG shadows.
This blocks an important optimization for mutation observers
(In reply to comment #9) > IIRC isShadowRoot() works for both new-style shadows and SVG shadows. No it doesn't; look at the definition of isShadowRoot. Adam: Any reason not to R? this patch. It looks good to me.
(In reply to comment #11) > (In reply to comment #9) > > IIRC isShadowRoot() works for both new-style shadows and SVG shadows. > > No it doesn't; look at the definition of isShadowRoot. > > Adam: Any reason not to R? this patch. It looks good to me. Only Roland's comments, pushed back to R?.
Comment on attachment 106525 [details] Patch Clearing flags on attachment: 106525 Committed r98275: <http://trac.webkit.org/changeset/98275>
All reviewed patches have been landed. Closing bug.
(In reply to comment #11) > (In reply to comment #9) > > IIRC isShadowRoot() works for both new-style shadows and SVG shadows. > > No it doesn't; look at the definition of isShadowRoot. That definition has changed since I made the comment! And I wonder if we haven't introduced a bug here: If I reconstruct this correctly, isInShadowTree is used in SVG at least by SVGTRefElement. This used to climb the tree looking for isShadowRoot, which used to be set for both SVG and new-style shadow roots. With the changes that isShadowRoot no longer applies for SVG and the shortcut introduced here, this is probably broken (?).
(In reply to comment #15) > (In reply to comment #11) > > (In reply to comment #9) > > > IIRC isShadowRoot() works for both new-style shadows and SVG shadows. > > > > No it doesn't; look at the definition of isShadowRoot. > > That definition has changed since I made the comment! Can you point to when it changed? I don't think it has. > And I wonder if we haven't introduced a bug here: If there a bug, it was probably introduced in this revision: <http://trac.webkit.org/changeset/84225> so maybe we should re-review that. > If I reconstruct this correctly, isInShadowTree is used in SVG at least by SVGTRefElement. This used to climb the tree looking for isShadowRoot, which used to be set for both SVG and new-style shadow roots. With the changes that isShadowRoot no longer applies for SVG and the shortcut introduced here, this is probably broken (?). Yes, it looks broken to me. (Looks like I broke it.)