Bug 67686 – Optimize Node::isInShadowTree to execute in constant-time

• Component: New Bugs
• Status: RESOLVED FIXED

[Adam Klein]

Optimize Node::isInShadowTree to execute in constant-time

[Adam Klein]

Created attachment 106525 [details]
Patch

[Dominic Cooney]

Comment on attachment 106525 [details]
Patch

Awesome.

[Adam Klein]

I should ask: do y'all know of a particular test that might exercise isInShadowTree()?  There are only a few callers...

[Dimitri Glazkov (Google)]

(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.

[Adam Klein]

(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...

[Roland Steiner]

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.

[Dominic Cooney]

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.

[Dimitri Glazkov (Google)]

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?

[Roland Steiner]

(In reply to comment #8)

IIRC isShadowRoot() works for both new-style shadows and SVG shadows.

[Rafael Weinstein]

This blocks an important optimization for mutation observers

[Dominic Cooney]

(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.

[Adam Klein]

(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?.

[WebKit Review Bot]

Comment on attachment 106525 [details]
Patch

Clearing flags on attachment: 106525

Committed r98275: <http://trac.webkit.org/changeset/98275>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Roland Steiner]

(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 (?).

[Dominic Cooney]

(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.)