We can avoid traversing DOM tree up to find canvas/form elements.
Created attachment 420921 [details] WIP
Created attachment 421386 [details] WIP2 - alternative approach
<rdar://problem/74680265>
Created attachment 421490 [details] Patch
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()?
(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.
Created attachment 421508 [details] Updated for trunk
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.
(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.
Thanks for the reviews, Antti & Simon!
> > 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.
> 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.
(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<>(~))
(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.
Committed r273479 (234558@main): <https://commits.webkit.org/234558@main>
> 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
Re-opened since this is blocked by bug 222510
This was not an overall speed up after all.