Bug 222159 – Avoid tree traversals to look for form and canvas elements

• Component: DOM
• Status: RESOLVED WONTFIX

[Ryosuke Niwa]

We can avoid traversing DOM tree up to find canvas/form elements.

[Ryosuke Niwa]

Created attachment 420921 [details]
WIP

[Ryosuke Niwa]

Created attachment 421386 [details]
WIP2 - alternative approach

[Radar WebKit Bug Importer]

<rdar://problem/74680265>

[Ryosuke Niwa]

Created attachment 421490 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 421490 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=421490&action=review

> Source/WebCore/dom/Element.cpp:2404
> +    if (init.delegatesFocus)
> +        setDelegatesFocusToShadowRoot();

Would be nice to split these changes into another patch.

> Source/WebCore/dom/Node.h:927
> +    if (states.contains(AncestorState::Canvas))

I'd prefer a blank line above this.

> Source/WebCore/html/HTMLFormElement.cpp:915
> +    if (UNLIKELY(is<HTMLFormElement>(startElement))) {
> +        auto* parentElement = startElement.parentElement();
> +        if (!parentElement || !parentElement->inclusiveAncestorStates().contains(AncestorState::Form)) {
> +            ASSERT(!ancestorsOfType<HTMLFormElement>(startElement).first());
> +            return nullptr;
> +        }
> +    }

Why don't we add firstAncestorsOfType<> and use it everywhere we do ancestorsOfType<>().first()?

[Ryosuke Niwa]

(In reply to Simon Fraser (smfr) from comment #5)
> Comment on attachment 421490 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=421490&action=review
> 
> > Source/WebCore/dom/Element.cpp:2404
> > +    if (init.delegatesFocus)
> > +        setDelegatesFocusToShadowRoot();
> 
> Would be nice to split these changes into another patch.

Done that in the bug 222404.

> > Source/WebCore/dom/Node.h:927
> > +    if (states.contains(AncestorState::Canvas))
> 
> I'd prefer a blank line above this.

Done.

> > Source/WebCore/html/HTMLFormElement.cpp:915
> > +    if (UNLIKELY(is<HTMLFormElement>(startElement))) {
> > +        auto* parentElement = startElement.parentElement();
> > +        if (!parentElement || !parentElement->inclusiveAncestorStates().contains(AncestorState::Form)) {
> > +            ASSERT(!ancestorsOfType<HTMLFormElement>(startElement).first());
> > +            return nullptr;
> > +        }
> > +    }
> 
> Why don't we add firstAncestorsOfType<> and use it everywhere we do
> ancestorsOfType<>().first()?

That sounds like a good idea. Probably in a separate patch though.

[Ryosuke Niwa]

Created attachment 421508 [details]
Updated for trunk

[Antti Koivisto]

Comment on attachment 421508 [details]
Updated for trunk

View in context: https://bugs.webkit.org/attachment.cgi?id=421508&action=review

> Source/WebCore/html/HTMLFormElement.cpp:912
> +        if (!parentElement || !parentElement->inclusiveAncestorStates().contains(AncestorState::Form)) {
> +            ASSERT(!ancestorsOfType<HTMLFormElement>(startElement).first());

Maybe you should make ancestorsOfType and pals to do these checks (based on the type obv) so all future code benefits too.

[Ryosuke Niwa]

(In reply to Antti Koivisto from comment #8)
> Comment on attachment 421508 [details]
> Updated for trunk
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=421508&action=review
> 
> > Source/WebCore/html/HTMLFormElement.cpp:912
> > +        if (!parentElement || !parentElement->inclusiveAncestorStates().contains(AncestorState::Form)) {
> > +            ASSERT(!ancestorsOfType<HTMLFormElement>(startElement).first());
> 
> Maybe you should make ancestorsOfType and pals to do these checks (based on
> the type obv) so all future code benefits too.

Oh you mean as a type specialization for HTMLFormElement? That might be a neat idea. If we spot a few more of these, it might be worth doing.

[Ryosuke Niwa]

Thanks for the reviews, Antti & Simon!

[Antti Koivisto]

> > Why don't we add firstAncestorsOfType<> and use it everywhere we do
> > ancestorsOfType<>().first()?
> 
> That sounds like a good idea. Probably in a separate patch though.

Please don't increase complexity for no gain. That literally just saves one '.' of typing.

[Antti Koivisto]

> Oh you mean as a type specialization for HTMLFormElement? That might be a
> neat idea. If we spot a few more of these, it might be worth doing.

Or maybe have a static field that gives the bit to test.

[Ryosuke Niwa]

(In reply to Antti Koivisto from comment #11)
> > > Why don't we add firstAncestorsOfType<> and use it everywhere we do
> > > ancestorsOfType<>().first()?
> > 
> > That sounds like a good idea. Probably in a separate patch though.
> 
> Please don't increase complexity for no gain. That literally just saves one
> '.' of typing.

I think it's more to do with the fact "first" comes at the end split with (~). Maybe what we need if something like firstIfExists(ancestorsOfType<>(~))

[Ryosuke Niwa]

(In reply to Antti Koivisto from comment #12)
> > Oh you mean as a type specialization for HTMLFormElement? That might be a
> > neat idea. If we spot a few more of these, it might be worth doing.
> 
> Or maybe have a static field that gives the bit to test.

Yeah, but I'm imagining it's a template specialization based on whether that bit is available or not. I agree we don't want to repeat all this code for each element type.

[Ryosuke Niwa]

Committed r273479 (234558@main): <https://commits.webkit.org/234558@main>

[Antti Koivisto]

> Yeah, but I'm imagining it's a template specialization based on whether that
> bit is available or not. I agree we don't want to repeat all this code for
> each element type.

if constexpr () is probably all that is needed

[WebKit Commit Bot]

Re-opened since this is blocked by bug 222510

[Ryosuke Niwa]

This was not an overall speed up after all.