[webkit-reviews] review granted: [Bug 189075] Modernize SlotAssignment : [Attachment 348392] Cleanup

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 189075: Modernize SlotAssignment
https://bugs.webkit.org/show_bug.cgi?id=189075

Attachment 348392: Cleanup

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




--- Comment #3 from Antti Koivisto <koivisto at iki.fi> ---
Comment on attachment 348392
  --> https://bugs.webkit.org/attachment.cgi?id=348392
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.