Bug 229748 – Eagerly resolve slot elements to simply the code in SlotAssignment

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Ryosuke Niwa]

We have a rather elaborate logic to lazily resolve slot elements when there are multiple ones of the same name.

Get rid of this code since that's a very rare case, and it doesn't really benefit perf much
since the only case we'd get faster is when scripts inserts & removes slot elements repeatedly.

[Ryosuke Niwa]

Actually, it's even more stupid. It only benefits details element since whenever slot change may be needed, we'd go through the slow path anyway. This is really stupid.

[Ryosuke Niwa]

Created attachment 436980 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 436980 [details]
Patch

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

> Source/WebCore/dom/SlotAssignment.cpp:-125
> -    if (!m_slotAssignmentsIsValid)
> -        assignSlots(shadowRoot);

FYI, this code was totally unnecessary. hasAssignedNodes calls it internally where it's needed
In all other cases hasAssignedNodes is not called, we're just waiting CPU cycle traversing through nodes.

[Ryosuke Niwa]

Ugh... I need to special case when we're in the middle of shadow root teardown.

[Ryosuke Niwa]

Created attachment 437003 [details]
Fixed the crash

[Chris Dumez]

Comment on attachment 437003 [details]
Fixed the crash

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

> Source/WebCore/ChangeLog:8
> +        This patch makes the resolution of slot elements eager. Lazily resolution stopping making any sense

stopping -> stopped?

> Source/WebCore/ChangeLog:11
> +        Right now, this lazy optimzation only applies when scripts repeatedly inserts & removes a slot element

Typo: optimzation

> Source/WebCore/ChangeLog:13
> +        and there are assigned nodes to the slot. There is no reason to overcomplicate the slot assignemnt code

typo: assignemnt

[Ryosuke Niwa]

Will address these. Thanks!

(In reply to Chris Dumez from comment #6)
> Comment on attachment 437003 [details]
> Fixed the crash
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=437003&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        This patch makes the resolution of slot elements eager. Lazily resolution stopping making any sense
> 
> stopping -> stopped?
> 
> > Source/WebCore/ChangeLog:11
> > +        Right now, this lazy optimzation only applies when scripts repeatedly inserts & removes a slot element
> 
> Typo: optimzation
> 
> > Source/WebCore/ChangeLog:13
> > +        and there are assigned nodes to the slot. There is no reason to overcomplicate the slot assignemnt code
> 
> typo: assignemnt

[Ryosuke Niwa]

Committed r281878 (241208@main): <https://commits.webkit.org/241208@main>

[Radar WebKit Bug Importer]

<rdar://problem/82646116>

[Ryosuke Niwa]

(In reply to Ryosuke Niwa from comment #3)
> Comment on attachment 436980 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=436980&action=review
> 
> > Source/WebCore/dom/SlotAssignment.cpp:-125
> > -    if (!m_slotAssignmentsIsValid)
> > -        assignSlots(shadowRoot);
> 
> FYI, this code was totally unnecessary. hasAssignedNodes calls it internally
> where it's needed
> In all other cases hasAssignedNodes is not called, we're just waiting CPU
> cycle traversing through nodes.

This was totally wrong LOL.

[Ryosuke Niwa]

Committed r282120 (241417@main): <https://commits.webkit.org/241417@main>