Summary: | Modernize SlotAssignment | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||
Component: | New Bugs | Assignee: | Ryosuke Niwa <rniwa> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | cdumez, commit-queue, darin, dbates, esprehn+autocc, ews-watchlist, gyuyoung.kim, kangil.han, koivisto, sam, webkit-bug-importer | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 148695 | ||||||||
Attachments: |
|
Description
Ryosuke Niwa
2018-08-28 22:23:16 PDT
Created attachment 348392 [details]
Cleanup
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. (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! Created attachment 348427 [details]
Patch for landing
Comment on attachment 348427 [details]
Patch for landing
Wait for EWS.
Committed r235483: <https://trac.webkit.org/changeset/235483> |