Bug 189075 – Modernize SlotAssignment

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Ryosuke Niwa]

Modernize SlotAssignment

[Ryosuke Niwa]

Created attachment 348392 [details]
Cleanup

[Radar WebKit Bug Importer]

<rdar://problem/43829595>

[Antti Koivisto]

Comment on attachment 348392 [details]
Cleanup

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

> Source/WebCore/dom/SlotAssignment.cpp:87
>      const AtomicString& slotName = slotNameFromAttributeValue(name);
> -    auto addResult = m_slots.add(slotName, std::unique_ptr<SlotInfo>());
> +    auto addResult = m_slots.ensure(slotName, [&slotElement] {
> +        return std::make_unique<Slot>(makeWeakPtr(slotElement));
> +    });
>      if (addResult.isNewEntry) {
> -        addResult.iterator->value = std::make_unique<SlotInfo>(slotElement);
> -        if (slotName == defaultSlotName()) // Because assignSlots doesn't collect nodes assigned to the default slot as an optimzation.
> +        // Unlike named slots, assignSlots doesn't collect nodes assigned to the default slot
> +        // to avoid always having a vector of all child nodes of a shadow host.
> +        if (slotName == defaultSlotName())
>              m_slotAssignmentsIsValid = false;
>          return;
>      }

Alternative factoring:

- Call plain constructor for Slot in ensure (just std::make_unique<Slot>(), you can remove the the slot element constructor)
- Move the remaining code in addResult.isNewEntry branch to the ensure lambda
- Remove the early return and let the code below handle setting the slot element and incrementing the count. It seems to do the right thing.

I think this would make the logic simpler and easier to follow.

[Ryosuke Niwa]

(In reply to Antti Koivisto from comment #3)
> Comment on attachment 348392 [details]
> Cleanup
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=348392&action=review
> 
> > Source/WebCore/dom/SlotAssignment.cpp:87
> >      const AtomicString& slotName = slotNameFromAttributeValue(name);
> > -    auto addResult = m_slots.add(slotName, std::unique_ptr<SlotInfo>());
> > +    auto addResult = m_slots.ensure(slotName, [&slotElement] {
> > +        return std::make_unique<Slot>(makeWeakPtr(slotElement));
> > +    });
> >      if (addResult.isNewEntry) {
> > -        addResult.iterator->value = std::make_unique<SlotInfo>(slotElement);
> > -        if (slotName == defaultSlotName()) // Because assignSlots doesn't collect nodes assigned to the default slot as an optimzation.
> > +        // Unlike named slots, assignSlots doesn't collect nodes assigned to the default slot
> > +        // to avoid always having a vector of all child nodes of a shadow host.
> > +        if (slotName == defaultSlotName())
> >              m_slotAssignmentsIsValid = false;
> >          return;
> >      }
> 
> Alternative factoring:
> 
> - Call plain constructor for Slot in ensure (just std::make_unique<Slot>(),
> you can remove the the slot element constructor)
> - Move the remaining code in addResult.isNewEntry branch to the ensure lambda
> - Remove the early return and let the code below handle setting the slot
> element and incrementing the count. It seems to do the right thing.
> 
> I think this would make the logic simpler and easier to follow.

Sure, will do that. Thanks for the review!

[Ryosuke Niwa]

Created attachment 348427 [details]
Patch for landing

[Ryosuke Niwa]

Comment on attachment 348427 [details]
Patch for landing

Wait for EWS.

[Ryosuke Niwa]

Committed r235483: <https://trac.webkit.org/changeset/235483>