Bug 189075 - Modernize SlotAssignment
Summary: Modernize SlotAssignment
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar
Depends on:
Blocks: 148695
  Show dependency treegraph
 
Reported: 2018-08-28 22:23 PDT by Ryosuke Niwa
Modified: 2018-08-29 15:23 PDT (History)
11 users (show)

See Also:


Attachments
Cleanup (10.66 KB, patch)
2018-08-28 22:28 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Patch for landing (10.72 KB, patch)
2018-08-29 13:40 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2018-08-28 22:23:16 PDT
Modernize SlotAssignment
Comment 1 Ryosuke Niwa 2018-08-28 22:28:06 PDT
Created attachment 348392 [details]
Cleanup
Comment 2 Radar WebKit Bug Importer 2018-08-28 22:39:30 PDT
<rdar://problem/43829595>
Comment 3 Antti Koivisto 2018-08-29 03:36:36 PDT
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.
Comment 4 Ryosuke Niwa 2018-08-29 13:38:46 PDT
(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!
Comment 5 Ryosuke Niwa 2018-08-29 13:40:09 PDT
Created attachment 348427 [details]
Patch for landing
Comment 6 Ryosuke Niwa 2018-08-29 13:40:37 PDT
Comment on attachment 348427 [details]
Patch for landing

Wait for EWS.
Comment 7 Ryosuke Niwa 2018-08-29 15:23:32 PDT
Committed r235483: <https://trac.webkit.org/changeset/235483>