Modified: trunk/Source/WebCore/ChangeLog (281877 => 281878)
--- trunk/Source/WebCore/ChangeLog 2021-09-01 21:28:01 UTC (rev 281877)
+++ trunk/Source/WebCore/ChangeLog 2021-09-01 21:30:55 UTC (rev 281878)
@@ -1,3 +1,45 @@
+2021-09-01 Ryosuke Niwa <[email protected]>
+
+ Eagerly resolve slot elements to simply the code in SlotAssignment
+ https://bugs.webkit.org/show_bug.cgi?id=229748
+
+ Reviewed by Chris Dumez.
+
+ This patch makes the resolution of slot elements eager. Lazily resolution stopped making any sense
+ once slotchange event was required whenever a slot element was inserted or removed per r235650.
+
+ Right now, this lazy optimization only applies when scripts repeatedly inserts & removes a slot element
+ in such a manner that there are multiple slot elements of the same name in a single shadow tree,
+ and there are assigned nodes to the slot. There is no reason to overcomplicate the slot assignment code
+ for this insane edge case.
+
+ No new tests since there should be no observable behavior change.
+
+ * dom/ShadowRoot.cpp:
+ (WebCore::ShadowRoot::findAssignedSlot):
+ * dom/SlotAssignment.cpp:
+ (WebCore::SlotAssignment::findAssignedSlot):
+ (WebCore::SlotAssignment::addSlotElementByName): Always call resolveSlotsAfterSlotMutation when there
+ is an ambiguity as to whether this is the first slot element or not.
+ (WebCore::SlotAssignment::removeSlotElementByName): Ditto for removal case.
+ (WebCore::SlotAssignment::resolveSlotsAfterSlotMutation):
+ (WebCore::SlotAssignment::resolveSlotsBeforeNodeInsertionOrRemoval): Moved to the header file.
+ (WebCore::SlotAssignment::willRemoveAllChildren): Ditto.
+ (WebCore::SlotAssignment::didChangeSlot):
+ (WebCore::SlotAssignment::assignedNodesForSlot):
+ (WebCore::SlotAssignment::findFirstSlotElement):
+ (WebCore::SlotAssignment::resolveAllSlotElements): Deleted.
+ * dom/SlotAssignment.h:
+ (WebCore::SlotAssignment::resolveSlotsBeforeNodeInsertionOrRemoval): Moved from cpp file now that it
+ doesn't optionally call resolveAllSlotElements. I'm not renaming this function for now since I want to
+ remove it altogether in a followup.
+ (WebCore::SlotAssignment::willRemoveAllChildren): Ditto.
+ (WebCore::ShadowRoot::resolveSlotsBeforeNodeInsertionOrRemoval): Always update the slot mutation version
+ and clear m_willBeRemovingAllChildren for simplicity. This extra condition will only apply for
+ the deatils element, which nobody cares about it. Meanwhile, we're wasting many IC entries for all other
+ node types whenever a node is inserted or removed. It's a pure madness.
+ (WebCore::ShadowRoot::willRemoveAllChildren): Ditto.
+
2021-09-01 Jean-Yves Avenard <[email protected]>
'Show Next Frame' and 'Show Previous Frame' keyboard shortcuts seem out of context and only pause video
Modified: trunk/Source/WebCore/dom/ShadowRoot.cpp (281877 => 281878)
--- trunk/Source/WebCore/dom/ShadowRoot.cpp 2021-09-01 21:28:01 UTC (rev 281877)
+++ trunk/Source/WebCore/dom/ShadowRoot.cpp 2021-09-01 21:30:55 UTC (rev 281878)
@@ -226,7 +226,7 @@
ASSERT(node.parentNode() == host());
if (!m_slotAssignment)
return nullptr;
- return m_slotAssignment->findAssignedSlot(node, *this);
+ return m_slotAssignment->findAssignedSlot(node);
}
void ShadowRoot::renameSlotElement(HTMLSlotElement& slot, const AtomString& oldName, const AtomString& newName)
Modified: trunk/Source/WebCore/dom/SlotAssignment.cpp (281877 => 281878)
--- trunk/Source/WebCore/dom/SlotAssignment.cpp 2021-09-01 21:28:01 UTC (rev 281877)
+++ trunk/Source/WebCore/dom/SlotAssignment.cpp 2021-09-01 21:30:55 UTC (rev 281878)
@@ -75,7 +75,7 @@
SlotAssignment::~SlotAssignment() = default;
-HTMLSlotElement* SlotAssignment::findAssignedSlot(const Node& node, ShadowRoot& shadowRoot)
+HTMLSlotElement* SlotAssignment::findAssignedSlot(const Node& node)
{
if (!is<Text>(node) && !is<Element>(node))
return nullptr;
@@ -84,7 +84,7 @@
if (!slot)
return nullptr;
- return findFirstSlotElement(*slot, shadowRoot);
+ return findFirstSlotElement(*slot);
}
inline bool SlotAssignment::hasAssignedNodes(ShadowRoot& shadowRoot, Slot& slot)
@@ -114,6 +114,10 @@
// FIXME: We should be able to do a targeted reconstruction.
shadowRoot.host()->invalidateStyleAndRenderersForSubtree();
+ if (!m_slotElementCount)
+ shadowRoot.host()->setHasShadowRootContainingSlots(true);
+ m_slotElementCount++;
+
auto& slotName = slotNameFromAttributeValue(name);
auto addResult = m_slots.ensure(slotName, [&] {
m_slotAssignmentsIsValid = false;
@@ -121,29 +125,14 @@
});
auto& slot = *addResult.iterator->value;
- if (!m_slotAssignmentsIsValid)
- assignSlots(shadowRoot);
-
- shadowRoot.host()->setHasShadowRootContainingSlots(true);
- m_slotElementCount++;
-
- bool needsSlotchangeEvent = shadowRoot.shouldFireSlotchangeEvent() && hasAssignedNodes(shadowRoot, slot);
-
slot.elementCount++;
if (slot.elementCount == 1) {
slot.element = makeWeakPtr(slotElement);
- if (needsSlotchangeEvent)
+ if (shadowRoot.shouldFireSlotchangeEvent() && hasAssignedNodes(shadowRoot, slot))
slotElement.enqueueSlotChangeEvent();
return;
}
- if (!needsSlotchangeEvent) {
- ASSERT(slot.element || m_needsToResolveSlotElements);
- slot.element = nullptr;
- m_needsToResolveSlotElements = true;
- return;
- }
-
resolveSlotsAfterSlotMutation(shadowRoot, SlotMutationType::Insertion);
}
@@ -166,35 +155,30 @@
auto* slot = m_slots.get(slotNameFromAttributeValue(name));
RELEASE_ASSERT(slot && slot->hasSlotElements());
- bool needsSlotchangeEvent = shadowRoot.shouldFireSlotchangeEvent() && hasAssignedNodes(shadowRoot, *slot);
slot->elementCount--;
if (!slot->elementCount) {
slot->element = nullptr;
- if (needsSlotchangeEvent && m_slotResolutionVersion != m_slotMutationVersion)
+ bool hasNotResolvedAllSlots = m_slotResolutionVersion != m_slotMutationVersion;
+ if (shadowRoot.shouldFireSlotchangeEvent() && hasAssignedNodes(shadowRoot, *slot) && hasNotResolvedAllSlots)
slotElement.enqueueSlotChangeEvent();
return;
}
- if (!needsSlotchangeEvent) {
- ASSERT(slot->element || m_needsToResolveSlotElements);
- slot->element = nullptr;
- m_needsToResolveSlotElements = true;
- return;
- }
-
bool elementWasRenamed = !oldParentOfRemovedTreeForRemoval;
if (elementWasRenamed && slot->element == &slotElement)
slotElement.enqueueSlotChangeEvent();
- // A previous invocation to resolveSlotsAfterSlotMutation during this removal has updated this slot.
- ASSERT(slot->element || (m_slotResolutionVersion == m_slotMutationVersion && !findSlotElement(shadowRoot, name)));
if (slot->element) {
resolveSlotsAfterSlotMutation(shadowRoot, elementWasRenamed ? SlotMutationType::Insertion : SlotMutationType::Removal,
m_willBeRemovingAllChildren ? oldParentOfRemovedTreeForRemoval : nullptr);
+ } else {
+ // A previous invocation to resolveSlotsAfterSlotMutation during this removal has updated this slot.
+ ASSERT(m_slotResolutionVersion == m_slotMutationVersion && !findSlotElement(shadowRoot, name));
}
if (slot->oldElement == &slotElement) {
+ ASSERT(shadowRoot.shouldFireSlotchangeEvent());
slotElement.enqueueSlotChangeEvent();
slot->oldElement = nullptr;
}
@@ -207,7 +191,6 @@
m_slotResolutionVersion = m_slotMutationVersion;
ASSERT(!subtreeToSkip || mutationType == SlotMutationType::Removal);
- m_needsToResolveSlotElements = false;
for (auto& slot : m_slots.values())
slot->seenFirstElement = false;
@@ -226,6 +209,7 @@
}
if (currentSlot->seenFirstElement) {
if (mutationType == SlotMutationType::Insertion && currentSlot->oldElement == currentElement) {
+ ASSERT(shadowRoot.shouldFireSlotchangeEvent());
currentElement->enqueueSlotChangeEvent();
currentSlot->oldElement = nullptr;
}
@@ -233,9 +217,8 @@
}
currentSlot->seenFirstElement = true;
slotCount++;
- ASSERT(currentSlot->element || !hasAssignedNodes(shadowRoot, *currentSlot));
if (currentSlot->element != currentElement) {
- if (hasAssignedNodes(shadowRoot, *currentSlot)) {
+ if (shadowRoot.shouldFireSlotchangeEvent() && hasAssignedNodes(shadowRoot, *currentSlot)) {
currentSlot->oldElement = WTFMove(currentSlot->element);
currentElement->enqueueSlotChangeEvent();
}
@@ -283,23 +266,6 @@
slotElement.enqueueSlotChangeEvent();
}
-void SlotAssignment::resolveSlotsBeforeNodeInsertionOrRemoval(ShadowRoot& shadowRoot)
-{
- ASSERT(shadowRoot.shouldFireSlotchangeEvent());
- m_slotMutationVersion++;
- m_willBeRemovingAllChildren = false;
- if (m_needsToResolveSlotElements)
- resolveAllSlotElements(shadowRoot);
-}
-
-void SlotAssignment::willRemoveAllChildren(ShadowRoot& shadowRoot)
-{
- m_slotMutationVersion++;
- m_willBeRemovingAllChildren = true;
- if (m_needsToResolveSlotElements)
- resolveAllSlotElements(shadowRoot);
-}
-
void SlotAssignment::didChangeSlot(const AtomString& slotAttrValue, ShadowRoot& shadowRoot)
{
auto& slotName = slotNameFromAttributeValue(slotAttrValue);
@@ -313,7 +279,7 @@
slot->assignedNodes.clear();
m_slotAssignmentsIsValid = false;
- auto slotElement = makeRefPtr(findFirstSlotElement(*slot, shadowRoot));
+ auto slotElement = makeRefPtr(findFirstSlotElement(*slot));
if (!slotElement)
return;
@@ -344,7 +310,7 @@
return nullptr;
RELEASE_ASSERT(slot->hasSlotElements());
- if (slot->hasDuplicatedSlotElements() && findFirstSlotElement(*slot, shadowRoot) != &slotElement)
+ if (slot->hasDuplicatedSlotElements() && findFirstSlotElement(*slot) != &slotElement)
return nullptr;
return &slot->assignedNodes;
@@ -372,10 +338,9 @@
return slotNameFromSlotAttribute(child);
}
-HTMLSlotElement* SlotAssignment::findFirstSlotElement(Slot& slot, ShadowRoot& shadowRoot)
+HTMLSlotElement* SlotAssignment::findFirstSlotElement(Slot& slot)
{
- if (slot.shouldResolveSlotElement())
- resolveAllSlotElements(shadowRoot);
+ RELEASE_ASSERT(!slot.shouldResolveSlotElement());
ASSERT(!slot.element || m_slotElementsForConsistencyCheck.contains(slot.element.get()));
ASSERT(!!slot.element == !!slot.elementCount);
@@ -383,33 +348,6 @@
return slot.element.get();
}
-void SlotAssignment::resolveAllSlotElements(ShadowRoot& shadowRoot)
-{
- ASSERT(m_needsToResolveSlotElements);
- m_needsToResolveSlotElements = false;
-
- // FIXME: It's inefficient to reset all values. We should be able to void this in common case.
- for (auto& entry : m_slots)
- entry.value->seenFirstElement = false;
-
- unsigned slotCount = m_slots.size();
- for (auto& slotElement : descendantsOfType<HTMLSlotElement>(shadowRoot)) {
- auto& slotName = slotNameFromAttributeValue(slotElement.attributeWithoutSynchronization(nameAttr));
-
- auto* slot = m_slots.get(slotName);
- RELEASE_ASSERT(slot); // slot must have been created when a slot was inserted.
-
- if (slot->seenFirstElement)
- continue;
- slot->seenFirstElement = true;
-
- slot->element = makeWeakPtr(slotElement);
- slotCount--;
- if (!slotCount)
- break;
- }
-}
-
void SlotAssignment::assignSlots(ShadowRoot& shadowRoot)
{
ASSERT(!m_slotAssignmentsIsValid);
Modified: trunk/Source/WebCore/dom/SlotAssignment.h (281877 => 281878)
--- trunk/Source/WebCore/dom/SlotAssignment.h 2021-09-01 21:28:01 UTC (rev 281877)
+++ trunk/Source/WebCore/dom/SlotAssignment.h 2021-09-01 21:30:55 UTC (rev 281878)
@@ -48,15 +48,16 @@
static const AtomString& defaultSlotName() { return emptyAtom(); }
- HTMLSlotElement* findAssignedSlot(const Node&, ShadowRoot&);
+ HTMLSlotElement* findAssignedSlot(const Node&);
void renameSlotElement(HTMLSlotElement&, const AtomString& oldName, const AtomString& newName, ShadowRoot&);
void addSlotElementByName(const AtomString&, HTMLSlotElement&, ShadowRoot&);
void removeSlotElementByName(const AtomString&, HTMLSlotElement&, ContainerNode* oldParentOfRemovedTreeForRemoval, ShadowRoot&);
void slotFallbackDidChange(HTMLSlotElement&, ShadowRoot&);
- void resolveSlotsBeforeNodeInsertionOrRemoval(ShadowRoot&);
- void willRemoveAllChildren(ShadowRoot&);
+ void resolveSlotsBeforeNodeInsertionOrRemoval();
+ void willRemoveAllChildren();
+
void didChangeSlot(const AtomString&, ShadowRoot&);
const Vector<WeakPtr<Node>>* assignedNodesForSlot(const HTMLSlotElement&, ShadowRoot&);
@@ -75,9 +76,9 @@
bool shouldResolveSlotElement() { return !element && elementCount; }
WeakPtr<HTMLSlotElement> element;
- WeakPtr<HTMLSlotElement> oldElement;
+ WeakPtr<HTMLSlotElement> oldElement; // Set by resolveSlotsAfterSlotMutation to dispatch slotchange in tree order.
unsigned elementCount { 0 };
- bool seenFirstElement { false };
+ bool seenFirstElement { false }; // Used in resolveSlotsAfterSlotMutation.
Vector<WeakPtr<Node>> assignedNodes;
};
@@ -87,8 +88,7 @@
virtual const AtomString& slotNameForHostChild(const Node&) const;
- HTMLSlotElement* findFirstSlotElement(Slot&, ShadowRoot&);
- void resolveAllSlotElements(ShadowRoot&);
+ HTMLSlotElement* findFirstSlotElement(Slot&);
void assignSlots(ShadowRoot&);
void assignToSlot(Node& child, const AtomString& slotName);
@@ -99,7 +99,6 @@
HashSet<HTMLSlotElement*> m_slotElementsForConsistencyCheck;
#endif
- bool m_needsToResolveSlotElements { false };
bool m_slotAssignmentsIsValid { false };
bool m_willBeRemovingAllChildren { false };
unsigned m_slotMutationVersion { 0 };
@@ -107,16 +106,28 @@
unsigned m_slotElementCount { 0 };
};
+inline void SlotAssignment::resolveSlotsBeforeNodeInsertionOrRemoval()
+{
+ m_slotMutationVersion++;
+ m_willBeRemovingAllChildren = false;
+}
+
+inline void SlotAssignment::willRemoveAllChildren()
+{
+ m_slotMutationVersion++;
+ m_willBeRemovingAllChildren = true;
+}
+
inline void ShadowRoot::resolveSlotsBeforeNodeInsertionOrRemoval()
{
- if (UNLIKELY(shouldFireSlotchangeEvent() && m_slotAssignment))
- m_slotAssignment->resolveSlotsBeforeNodeInsertionOrRemoval(*this);
+ if (UNLIKELY(m_slotAssignment))
+ m_slotAssignment->resolveSlotsBeforeNodeInsertionOrRemoval();
}
inline void ShadowRoot::willRemoveAllChildren(ContainerNode&)
{
- if (UNLIKELY(shouldFireSlotchangeEvent() && m_slotAssignment))
- m_slotAssignment->willRemoveAllChildren(*this);
+ if (UNLIKELY(m_slotAssignment))
+ m_slotAssignment->willRemoveAllChildren();
}
inline void ShadowRoot::didRemoveAllChildrenOfShadowHost()
