We do a lot of unnecessary node change work, for example, updating a property on a node when that node itself was already updated. This should be de-duplicated.
<rdar://problem/119052949>
Created attachment 468837 [details] Patch
Created attachment 468853 [details] Patch
Created attachment 468900 [details] Patch
(In reply to Joshua Hoffman from comment #4) > Created attachment 468900 [details] > Patch diff --git a/Source/WebCore/accessibility/AXObjectCache.cpp b/Source/WebCore/accessibility/AXObjectCache.cpp index 0a75f73aecb2..775b709056ad 100644 --- a/Source/WebCore/accessibility/AXObjectCache.cpp +++ b/Source/WebCore/accessibility/AXObjectCache.cpp @@ -245,6 +245,7 @@ AXObjectCache::AXObjectCache(Document& document) , m_buildIsolatedTreeTimer(*this, &AXObjectCache::buildIsolatedTree) , m_geometryManager(AXGeometryManager::create(*this)) , m_selectedTextRangeTimer(*this, &AXObjectCache::selectedTextRangeTimerFired, platformSelectedTextRangeDebounceInterval()) + , m_updateTreeSnapshotTimer(*this, &AXObjectCache::updateTreeSnapshotTimerFired) #endif { AXTRACE(makeString("AXObjectCache::AXObjectCache 0x"_s, hex(reinterpret_cast<uintptr_t>(this)))); @@ -281,6 +282,7 @@ AXObjectCache::~AXObjectCache() #if ENABLE(ACCESSIBILITY_ISOLATED_TREE) m_selectedTextRangeTimer.stop(); + m_updateTreeSnapshotTimer.stop(); if (m_pageID) { if (auto tree = AXIsolatedTree::treeForPageID(*m_pageID)) @@ -4127,7 +4129,7 @@ void AXObjectCache::updateIsolatedTree(const Vector<std::pair<RefPtr<Accessibili auto updatedFields = updatedObjects.get(axObject->objectID()); if (!updatedFields.node) { updatedObjects.set(axObject->objectID(), UpdatedFields { updatedFields.children, true }); - tree->updateNode(*axObject); + tree->queueNodeUpdate(*axObject, { { }, true, false }); } }; @@ -4138,126 +4140,126 @@ void AXObjectCache::updateIsolatedTree(const Vector<std::pair<RefPtr<Accessibili switch (notification.second) { case AXAccessKeyChanged: - tree->updateNodeProperty(*notification.first, AXPropertyName::AccessKey); + tree->queueNodeUpdate(*notification.first, { AXPropertyName::AccessKey }); break; case AXAutofillTypeChanged: - tree->updateNodeProperty(*notification.first, AXPropertyName::ValueAutofillButtonType); + tree->queueNodeUpdate(*notification.first, { AXPropertyName::ValueAutofillButtonType }); break; case AXCellSlotsChanged: ASSERT(notification.first->isTable()); - tree->updateNodeProperty(*notification.first, AXPropertyName::CellSlots); + tree->queueNodeUpdate(*notification.first, { AXPropertyName::CellSlots }); break; case AXCheckedStateChanged: - tree->updateNodeProperty(*notification.first, AXPropertyName::IsChecked); + tree->queueNodeUpdate(*notification.first, { AXPropertyName::IsChecked }); break; case AXCurrentStateChanged: - tree->updateNodeProperty(*notification.first, AXPropertyName::CurrentState); + tree->queueNodeUpdate(*notification.first, { AXPropertyName::CurrentState }); break; case AXColumnCountChanged: - tree->updateNodeProperty(*notification.first, AXPropertyName::AXColumnCount); + tree->queueNodeUpdate(*notification.first, { AXPropertyName::AXColumnCount }); break; case AXColumnIndexChanged: - tree->updateNodeProperty(*notification.first, AXPropertyName::AXColumnIndex); + tree->queueNodeUpdate(*notification.first, { AXPropertyName::AXColumnIndex }); break; case AXColumnSpanChanged: - tree->updateNodeProperty(*notification.first, AXPropertyName::ColumnIndexRange); + tree->queueNodeUpdate(*notification.first, { AXPropertyName::ColumnIndexRange }); break; case AXContentEditableAttributeChanged: - tree->updateNodeProperty(*notification.first, AXPropertyName::IsNonNativeTextControl); + tree->queueNodeUpdate(*notification.first, { AXPropertyName::IsNonNativeTextControl }); break; case AXDisabledStateChanged: - tree->updatePropertiesForSelfAndDescendants(*notification.first, { AXPropertyName::CanSetFocusAttribute, AXPropertyName::IsEnabled }); + tree->updatePropertiesForSelfAndDescendants(*notification.first, { { AXPropertyName::CanSetFocusAttribute, AXPropertyName::IsEnabled } }); break; case AXExpandedChanged: - tree->updateNodeProperty(*notification.first, AXPropertyName::IsExpanded); + tree->queueNodeUpdate(*notification.first, { AXPropertyName::IsExpanded }); break; case AXExtendedDescriptionChanged: - tree->updateNodeProperty(*notification.first, AXPropertyName::ExtendedDescription); + tree->queueNodeUpdate(*notification.first, { AXPropertyName::ExtendedDescription }); break; case AXFocusableStateChanged: - tree->updateNodeProperty(*notification.first, AXPropertyName::CanSetFocusAttribute); + tree->queueNodeUpdate(*notification.first, { AXPropertyName::CanSetFocusAttribute }); break; case AXLabelCreated: tree->labelCreated(*notification.first); break; case AXMaximumValueChanged: - tree->updateNodeProperties(*notification.first, { AXPropertyName::MaxValueForRange, AXPropertyName::ValueForRange }); + tree->queueNodeUpdate(*notification.first, { { AXPropertyName::MaxValueForRange, AXPropertyName::ValueForRange } }); break; case AXMenuListItemSelected: { RefPtr ancestor = Accessibility::findAncestor<AccessibilityObject>(*notification.first, false, [] (const auto& object) { return object.isMenu() || object.isMenuBar(); }); if (ancestor) { - tree->updateNodeProperty(*ancestor, AXPropertyName::SelectedChildren); - tree->updateNodeProperty(*notification.first, AXPropertyName::IsSelected); + tree->queueNodeUpdate(*ancestor, { AXPropertyName::SelectedChildren }); + tree->queueNodeUpdate(*notification.first, { AXPropertyName::IsSelected }); } break; } case AXMinimumValueChanged: - tree->updateNodeProperties(*notification.first, { AXPropertyName::MinValueForRange, AXPropertyName::ValueForRange }); + tree->queueNodeUpdate(*notification.first, { { AXPropertyName::MinValueForRange, AXPropertyName::ValueForRange } }); break; case AXNameChanged: - tree->updateNodeProperty(*notification.first, AXPropertyName::NameAttribute); + tree->queueNodeUpdate(*notification.first, { AXPropertyName::NameAttribute }); break; case AXOrientationChanged: - tree->updateNodeProperty(*notification.first, AXPropertyName::Orientation); + tree->queueNodeUpdate(*notification.first, { AXPropertyName::Orientation }); break; case AXPositionInSetChanged: - tree->updateNodeProperties(*notification.first, { AXPropertyName::PosInSet, AXPropertyName::SupportsPosInSet }); + tree->queueNodeUpdate(*notification.first, { { AXPropertyName::PosInSet, AXPropertyName::SupportsPosInSet } }); break; case AXPopoverTargetChanged: - tree->updateNodeProperties(*notification.first, { AXPropertyName::SupportsExpanded, AXPropertyName::IsExpanded }); + tree->queueNodeUpdate(*notification.first, { { AXPropertyName::SupportsExpanded, AXPropertyName::IsExpanded } }); break; case AXSelectedTextChanged: - tree->updateNodeProperty(*notification.first, AXPropertyName::SelectedTextRange); + tree->queueNodeUpdate(*notification.first, { AXPropertyName::SelectedTextRange }); break; case AXSortDirectionChanged: - tree->updateNodeProperty(*notification.first, AXPropertyName::SortDirection); + tree->queueNodeUpdate(*notification.first, { AXPropertyName::SortDirection }); break; case AXIdAttributeChanged: - tree->updateNodeProperty(*notification.first, AXPropertyName::IdentifierAttribute); + tree->queueNodeUpdate(*notification.first, { AXPropertyName::IdentifierAttribute }); break; case AXReadOnlyStatusChanged: - tree->updateNodeProperty(*notification.first, AXPropertyName::CanSetValueAttribute); + tree->queueNodeUpdate(*notification.first, { AXPropertyName::CanSetValueAttribute }); break; case AXRequiredStatusChanged: - tree->updateNodeProperty(*notification.first, AXPropertyName::IsRequired); + tree->queueNodeUpdate(*notification.first, { AXPropertyName::IsRequired }); break; case AXRoleDescriptionChanged: - tree->updateNodeProperty(*notification.first, AXPropertyName::RoleDescription); + tree->queueNodeUpdate(*notification.first, { AXPropertyName::RoleDescription }); break; case AXRowIndexChanged: - tree->updateNodeProperty(*notification.first, AXPropertyName::AXRowIndex); + tree->queueNodeUpdate(*notification.first, { AXPropertyName::AXRowIndex }); break; case AXRowSpanChanged: - tree->updateNodeProperty(*notification.first, AXPropertyName::RowIndexRange); + tree->queueNodeUpdate(*notification.first, { AXPropertyName::RowIndexRange }); break; case AXCellScopeChanged: - tree->updateNodeProperties(*notification.first, { AXPropertyName::CellScope, AXPropertyName::IsColumnHeader, AXPropertyName::IsRowHeader }); + tree->queueNodeUpdate(*notification.first, { { AXPropertyName::CellScope, AXPropertyName::IsColumnHeader, AXPropertyName::IsRowHeader } }); break; // FIXME: Contrary to the name "AXSelectedCellsChanged", this notification can be posted on a cell // who has changed selected state, not just on table or grid who has changed its selected cells. case AXSelectedCellsChanged: case AXSelectedStateChanged: - tree->updateNodeProperty(*notification.first, AXPropertyName::IsSelected); + tree->queueNodeUpdate(*notification.first, { AXPropertyName::IsSelected }); break; case AXSetSizeChanged: - tree->updateNodeProperties(*notification.first, { AXPropertyName::SetSize, AXPropertyName::SupportsSetSize }); + tree->queueNodeUpdate(*notification.first, { { AXPropertyName::SetSize, AXPropertyName::SupportsSetSize } }); break; case AXTableHeadersChanged: - tree->updateNodeProperty(*notification.first, AXPropertyName::ColumnHeaders); + tree->queueNodeUpdate(*notification.first, { AXPropertyName::ColumnHeaders }); break; case AXTextCompositionChanged: - tree->updateNodeProperty(*notification.first, AXPropertyName::TextInputMarkedTextMarkerRange); + tree->queueNodeUpdate(*notification.first, { AXPropertyName::TextInputMarkedTextMarkerRange }); break; case AXURLChanged: - tree->updateNodeProperty(*notification.first, AXPropertyName::URL); + tree->queueNodeUpdate(*notification.first, { AXPropertyName::URL }); break; case AXKeyShortcutsChanged: - tree->updateNodeProperty(*notification.first, AXPropertyName::KeyShortcuts); + tree->queueNodeUpdate(*notification.first, { AXPropertyName::KeyShortcuts }); break; case AXVisibilityChanged: - tree->updateNodeProperty(*notification.first, AXPropertyName::IsVisible); + tree->queueNodeUpdate(*notification.first, { AXPropertyName::IsVisible }); break; case AXActiveDescendantChanged: case AXRoleChanged: @@ -4298,7 +4300,7 @@ void AXObjectCache::updateIsolatedTree(const Vector<std::pair<RefPtr<Accessibili auto updatedFields = updatedObjects.get(notification.first->objectID()); if (!updatedFields.children) { updatedObjects.set(notification.first->objectID(), UpdatedFields { true, updatedFields.node }); - tree->updateChildren(*notification.first); + tree->queueNodeUpdate(*notification.first, { { }, false, true }); } break; } @@ -4317,7 +4319,13 @@ void AXObjectCache::updateIsolatedTree(AccessibilityObject* axObject, AXProperty void AXObjectCache::updateIsolatedTree(AccessibilityObject& axObject, AXPropertyName property) const { if (RefPtr tree = AXIsolatedTree::treeForPageID(m_pageID)) - tree->updateNodeProperty(axObject, property); + tree->queueNodeUpdate(axObject, { property }); +} + +void AXObjectCache::startSnapshotTimer() +{ + if (!m_updateTreeSnapshotTimer.isActive()) + m_updateTreeSnapshotTimer.startOneShot(100_ms); AG: We should declare the elapse time as a const in the consts section of this file. } void AXObjectCache::onPaint(const RenderObject& renderer, IntRect&& paintRect) const @@ -4905,6 +4913,16 @@ void AXObjectCache::selectedTextRangeTimerFired() m_lastDebouncedTextRangeObject = { }; } + +void AXObjectCache::updateTreeSnapshotTimerFired() +{ + if (!accessibilityEnabled()) + return; + + m_updateTreeSnapshotTimer.stop(); + if (auto tree = getOrCreateIsolatedTree()) AG: use isolatedTreeForPageID. getOrCreateIsolatedTree should be used only in the paths that we purposely want to create the iso tree, namely, a client request for root or focus. + tree->processQueuedNodeUpdates(); +} #endif void AXObjectCache::onWidgetVisibilityChanged(RenderWidget* widget)
(In reply to Joshua Hoffman from comment #4) > Created attachment 468900 [details] > Patch diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp index 890a0ec4857c..44a394534ca2 100644 --- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp +++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp @@ -532,7 +532,7 @@ void AXIsolatedTree::updatePropertiesForSelfAndDescendants(AccessibilityObject& return; Accessibility::enumerateDescendants<AXCoreObject>(axObject, true, [&properties, this] (auto& descendant) { - updateNodeProperties(descendant, properties); + queueNodeUpdate(descendant, { properties }); }); } @@ -733,7 +733,7 @@ void AXIsolatedTree::updateNodeAndDependentProperties(AccessibilityObject& axObj bool updateTableAncestorColumns = is<AccessibilityTableRow>(axObject); for (auto* ancestor = axObject.parentObject(); ancestor; ancestor = ancestor->parentObject()) { if (ancestor->isTree()) { - updateNodeProperty(*ancestor, AXPropertyName::ARIATreeRows); + queueNodeUpdate(*ancestor, { AXPropertyName::ARIATreeRows }); if (!updateTableAncestorColumns) break; } @@ -990,7 +990,7 @@ void AXIsolatedTree::labelCreated(AccessibilityObject& axObject) { ASSERT(axObject.isLabel()); if (RefPtr correspondingControl = axObject.isLabel() ? axObject.correspondingControlForLabelElement() : nullptr) - updateNode(*correspondingControl); + queueNodeUpdate(*correspondingControl, { { }, true, false }); } void AXIsolatedTree::updateLoadingProgress(double newProgressValue) @@ -1238,6 +1238,67 @@ AXTreePtr findAXTree(Function<bool(AXTreePtr)>&& match) #endif } +void AXIsolatedTree::queueNodeUpdate(AXCoreObject& object, NodeUpdateOptions options) +{ + ASSERT(isMainThread()); + + // If we are updating a property, but our node is in m_needsUpdateNode, don't queue anything. + if (options.properties.size()) { + if (m_needsUpdateNode.contains(object.objectID())) + return; AG: what about if the caller passed { someProperties, true, false }, i.e., requested a node update and also passed some properties. + + Vector<AXPropertyName> properties; + if (m_needsPropertyUpdates.contains(object.objectID())) + properties = m_needsPropertyUpdates.get(object.objectID()); + + for (const auto& property : options.properties) + properties.appendIfNotContains(property); + + m_needsPropertyUpdates.set(object.objectID(), WTFMove(properties)); + } + + if (options.shouldUpdateChildren && !m_needsUpdateChildren.contains(object.objectID())) + m_needsUpdateChildren.add(object.objectID()); + + if (options.shouldUpdateNode && !m_needsUpdateNode.contains(object.objectID())) + m_needsUpdateNode.add(object.objectID()); + + if (auto* cache = axObjectCache()) + cache->startSnapshotTimer(); +} + +void AXIsolatedTree::processQueuedNodeUpdates() +{ + ASSERT(isMainThread()); + + auto* cache = axObjectCache(); + if (!cache) + return; + + for (AXID nodeID : m_needsUpdateChildren) { + if (auto* axObject = cache->objectForID(nodeID)) + updateChildren(*axObject, ResolveNodeChanges::No); + } + m_needsUpdateChildren.clear(); + + for (AXID nodeID : m_needsUpdateNode) { + if (!m_unresolvedPendingAppends.contains(nodeID)) + m_unresolvedPendingAppends.set(nodeID, AttachWrapper::OnMainThread); + } + m_needsUpdateNode.clear(); + + for (const auto& propertyUpdate : m_needsPropertyUpdates) { + if (m_unresolvedPendingAppends.contains(propertyUpdate.key)) + break; + + if (auto* axObject = cache->objectForID(propertyUpdate.key)) + updateNodeProperties(*axObject, propertyUpdate.value); + } + m_needsPropertyUpdates.clear(); + + queueRemovalsAndUnresolvedChanges({ }); +} + } // namespace WebCore #endif // ENABLE(ACCESSIBILITY_ISOLATED_TREE) diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h index c9609b8c8b1d..1bb2fd444ddc 100644 --- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h +++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h @@ -262,6 +262,26 @@ struct AXPropertyChange { AXPropertyMap properties; // Changed properties. }; +struct NodeUpdateOptions { + Vector<AXPropertyName> properties; + bool shouldUpdateNode { false }; + bool shouldUpdateChildren { false }; + + NodeUpdateOptions(Vector<AXPropertyName> propertyNames, bool shouldUpdateNode, bool shouldUpdateChildren) + : properties(propertyNames) + , shouldUpdateNode(shouldUpdateNode) + , shouldUpdateChildren(shouldUpdateChildren) + { } AG: pass the Vector by const& or &&. + + NodeUpdateOptions(Vector<AXPropertyName> propertyNames) + : properties(propertyNames) + { } AG: same here. Does the compileer require to explicitly define these "default" constructors? AG: does the order of the properties matter? or we could use a set instead of a vector? + + NodeUpdateOptions(AXPropertyName propertyName) + : properties({ propertyName }) + { } +}; + DECLARE_ALLOCATOR_WITH_HEAP_IDENTIFIER(AXIsolatedTree); class AXIsolatedTree : public ThreadSafeRefCountedAndCanMakeThreadSafeWeakPtr<AXIsolatedTree> , public AXTreeStore<AXIsolatedTree> { @@ -336,6 +356,9 @@ public: AXTextMarkerRange selectedTextMarkerRange(); void setSelectedTextMarkerRange(AXTextMarkerRange&&); + void queueNodeUpdate(AXCoreObject&, NodeUpdateOptions); AG: pass NodeUpdateOptions by const& or &&. + void processQueuedNodeUpdates(); + private: AXIsolatedTree(AXObjectCache&); static void storeTree(AXObjectCache&, const Ref<AXIsolatedTree>&); @@ -435,6 +458,11 @@ private: Lock m_changeLogLock; AXTextMarkerRange m_selectedTextMarkerRange WTF_GUARDED_BY_LOCK(m_changeLogLock); + + // Types of node updates (queued) + ListHashSet<AXID> m_needsUpdateChildren; + ListHashSet<AXID> m_needsUpdateNode; + HashMap<AXID, Vector<AXPropertyName>> m_needsPropertyUpdates; }; inline AXObjectCache* AXIsolatedTree::axObjectCache() const diff --git a/Source/WebCore/accessibility/mac/AXObjectCacheMac.mm b/Source/WebCore/accessibility/mac/AXObjectCacheMac.mm index 78d52e57e9ae..b511c7f00d35 100644 --- a/Source/WebCore/accessibility/mac/AXObjectCacheMac.mm +++ b/Source/WebCore/accessibility/mac/AXObjectCacheMac.mm @@ -325,6 +325,12 @@ void AXObjectCache::postPlatformNotification(AXCoreObject* object, AXNotificatio return; } +#if ENABLE(ACCESSIBILITY_ISOLATED_TREE) + // Process all pending node changes before sending notification. + if (auto tree = getOrCreateIsolatedTree()) AG: getOrCreateIsolatedTree -> treeByPageID + tree->processQueuedNodeUpdates(); +#endif + bool skipSystemNotification = false; // Some notifications are unique to Safari and do not have NSAccessibility equivalents. NSString *macNotification; @@ -463,6 +469,11 @@ void AXObjectCache::postPlatformAnnouncementNotification(const String& message) { ASSERT(isMainThread()); +#if ENABLE(ACCESSIBILITY_ISOLATED_TREE) + if (auto tree = getOrCreateIsolatedTree()) AG: same here. + tree->processQueuedNodeUpdates(); +#endif + NSDictionary *userInfo = @{ NSAccessibilityPriorityKey: @(NSAccessibilityPriorityHigh), NSAccessibilityAnnouncementKey: message, }; @@ -504,6 +515,11 @@ void AXObjectCache::postTextStateChangePlatformNotification(AccessibilityObject* return; RefPtr protectedObject = object; +#if ENABLE(ACCESSIBILITY_ISOLATED_TREE) + if (auto tree = getOrCreateIsolatedTree()) AG: same here. + tree->processQueuedNodeUpdates(); +#endif + auto userInfo = adoptNS([[NSMutableDictionary alloc] initWithCapacity:5]); if (m_isSynchronizingSelection) [userInfo setObject:@YES forKey:NSAccessibilityTextStateSyncKey]; @@ -624,6 +640,11 @@ void AXObjectCache::postTextReplacementPlatformNotification(AccessibilityObject* return; RefPtr protectedObject = object; +#if ENABLE(ACCESSIBILITY_ISOLATED_TREE) + if (auto tree = getOrCreateIsolatedTree()) AG: same here. + tree->processQueuedNodeUpdates(); +#endif + auto changes = adoptNS([[NSMutableArray alloc] initWithCapacity:2]); if (NSDictionary *change = textReplacementChangeDictionary(*object, deletionType, deletedText, position)) [changes addObject:change]; @@ -643,6 +664,11 @@ void AXObjectCache::postTextReplacementPlatformNotificationForTextControl(Access return; RefPtr protectedObject = object; +#if ENABLE(ACCESSIBILITY_ISOLATED_TREE) + if (auto tree = getOrCreateIsolatedTree()) AG: same here. + tree->processQueuedNodeUpdates(); +#endif + auto changes = adoptNS([[NSMutableArray alloc] initWithCapacity:2]); if (NSDictionary *change = textReplacementChangeDictionary(*object, AXTextEditTypeDelete, deletedText, textControl)) [changes addObject:change];
(In reply to Andres Gonzalez from comment #6) > (In reply to Joshua Hoffman from comment #4) > > Created attachment 468900 [details] > > Patch > > diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp > b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp > index 890a0ec4857c..44a394534ca2 100644 > --- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp > +++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp > @@ -532,7 +532,7 @@ void > AXIsolatedTree::updatePropertiesForSelfAndDescendants(AccessibilityObject& > return; > > Accessibility::enumerateDescendants<AXCoreObject>(axObject, true, > [&properties, this] (auto& descendant) { > - updateNodeProperties(descendant, properties); > + queueNodeUpdate(descendant, { properties }); > }); > } > > @@ -733,7 +733,7 @@ void > AXIsolatedTree::updateNodeAndDependentProperties(AccessibilityObject& axObj > bool updateTableAncestorColumns = is<AccessibilityTableRow>(axObject); > for (auto* ancestor = axObject.parentObject(); ancestor; ancestor = > ancestor->parentObject()) { > if (ancestor->isTree()) { > - updateNodeProperty(*ancestor, AXPropertyName::ARIATreeRows); > + queueNodeUpdate(*ancestor, { AXPropertyName::ARIATreeRows }); > if (!updateTableAncestorColumns) > break; > } > @@ -990,7 +990,7 @@ void AXIsolatedTree::labelCreated(AccessibilityObject& > axObject) > { > ASSERT(axObject.isLabel()); > if (RefPtr correspondingControl = axObject.isLabel() ? > axObject.correspondingControlForLabelElement() : nullptr) > - updateNode(*correspondingControl); > + queueNodeUpdate(*correspondingControl, { { }, true, false }); > } > > void AXIsolatedTree::updateLoadingProgress(double newProgressValue) > @@ -1238,6 +1238,67 @@ AXTreePtr findAXTree(Function<bool(AXTreePtr)>&& > match) > #endif > } > > +void AXIsolatedTree::queueNodeUpdate(AXCoreObject& object, > NodeUpdateOptions options) > +{ > + ASSERT(isMainThread()); > + > + // If we are updating a property, but our node is in m_needsUpdateNode, > don't queue anything. > + if (options.properties.size()) { > + if (m_needsUpdateNode.contains(object.objectID())) > + return; > > AG: what about if the caller passed { someProperties, true, false }, i.e., > requested a node update and also passed some properties. The callers shouldn't do this (since a node update completely invalidates the need to update properties), but this is handled/de-duped correctly with this code as-is. > + > + NodeUpdateOptions(Vector<AXPropertyName> propertyNames) > + : properties(propertyNames) > + { } > > AG: same here. Does the compileer require to explicitly define these > "default" constructors? When trying to get this to build properly I had to define all three constructors. I notice we define the default in the CharacterOffset struct as well. Have you encountered this before? > AG: does the order of the properties matter? or we could use a set instead > of a vector? We may be able to get away with a set here—I will give that a shot.
Created attachment 468908 [details] Patch
(In reply to Joshua Hoffman from comment #8) > Created attachment 468908 [details] > Patch diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp index 890a0ec4857c..ed8da3770da0 100644 --- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp +++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp @@ -531,8 +531,12 @@ void AXIsolatedTree::updatePropertiesForSelfAndDescendants(AccessibilityObject& if (isUpdatingSubtree()) return; - Accessibility::enumerateDescendants<AXCoreObject>(axObject, true, [&properties, this] (auto& descendant) { - updateNodeProperties(descendant, properties); + AXPropertyNameSet propertySet; + for (const auto& property : properties) + propertySet.add(property); + + Accessibility::enumerateDescendants<AXCoreObject>(axObject, true, [&propertySet, this] (auto& descendant) { + queueNodeUpdate(descendant, { propertySet }); }); } @@ -733,7 +737,7 @@ void AXIsolatedTree::updateNodeAndDependentProperties(AccessibilityObject& axObj bool updateTableAncestorColumns = is<AccessibilityTableRow>(axObject); for (auto* ancestor = axObject.parentObject(); ancestor; ancestor = ancestor->parentObject()) { if (ancestor->isTree()) { - updateNodeProperty(*ancestor, AXPropertyName::ARIATreeRows); + queueNodeUpdate(*ancestor, { AXPropertyName::ARIATreeRows }); if (!updateTableAncestorColumns) break; } @@ -990,7 +994,7 @@ void AXIsolatedTree::labelCreated(AccessibilityObject& axObject) { ASSERT(axObject.isLabel()); if (RefPtr correspondingControl = axObject.isLabel() ? axObject.correspondingControlForLabelElement() : nullptr) - updateNode(*correspondingControl); + queueNodeUpdate(*correspondingControl, { { }, true, false }); } void AXIsolatedTree::updateLoadingProgress(double newProgressValue) @@ -1238,6 +1242,67 @@ AXTreePtr findAXTree(Function<bool(AXTreePtr)>&& match) #endif } +void AXIsolatedTree::queueNodeUpdate(AXCoreObject& object, const NodeUpdateOptions& options) +{ + ASSERT(isMainThread()); + + if (!options.shouldUpdateNode && options.properties.size()) { + // If we are updating a property, but our node is already in m_needsUpdateNode, don't queue anything. + if (m_needsUpdateNode.contains(object.objectID())) + return; + + Vector<AXPropertyName> properties; + if (m_needsPropertyUpdates.contains(object.objectID())) + properties = m_needsPropertyUpdates.get(object.objectID()); + + for (const auto& property : options.properties) + properties.appendIfNotContains(property); + + m_needsPropertyUpdates.set(object.objectID(), WTFMove(properties)); + } + + if (options.shouldUpdateChildren && !m_needsUpdateChildren.contains(object.objectID())) + m_needsUpdateChildren.add(object.objectID()); + + if (options.shouldUpdateNode && !m_needsUpdateNode.contains(object.objectID())) + m_needsUpdateNode.add(object.objectID()); AG: Don't need to do && !m_needsUpdateNode.contains(object.objectID()) since the add method handles correctly whether the entry exists or not. + + if (auto* cache = axObjectCache()) + cache->startSnapshotTimer(); +} + +void AXIsolatedTree::processQueuedNodeUpdates() +{ + ASSERT(isMainThread()); + + auto* cache = axObjectCache(); + if (!cache) + return; + + for (AXID nodeID : m_needsUpdateChildren) { + if (auto* axObject = cache->objectForID(nodeID)) + updateChildren(*axObject, ResolveNodeChanges::No); + } + m_needsUpdateChildren.clear(); + + for (AXID nodeID : m_needsUpdateNode) { AG: we use objectID not nodeID, although in other naming we mix them. My preference is to reserve node naming for DOM Nodes and use object for AX objects. + if (!m_unresolvedPendingAppends.contains(nodeID)) + m_unresolvedPendingAppends.set(nodeID, AttachWrapper::OnMainThread); AG: Can we attach the wrapper on the main thread? I think that if we are replacing an existing object, we cannot. + } + m_needsUpdateNode.clear(); + + for (const auto& propertyUpdate : m_needsPropertyUpdates) { + if (m_unresolvedPendingAppends.contains(propertyUpdate.key)) + break; + + if (auto* axObject = cache->objectForID(propertyUpdate.key)) + updateNodeProperties(*axObject, propertyUpdate.value); + } + m_needsPropertyUpdates.clear(); + + queueRemovalsAndUnresolvedChanges({ }); +} + } // namespace WebCore #endif // ENABLE(ACCESSIBILITY_ISOLATED_TREE) diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h index c9609b8c8b1d..176f5d589096 100644 --- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h +++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h @@ -246,6 +246,8 @@ enum class AXPropertyName : uint16_t { VisibleRows, }; +using AXPropertyNameSet = HashSet<AXPropertyName, IntHash<AXPropertyName>, WTF::StrongEnumHashTraits<AXPropertyName>>; + // If this type is modified, the switchOn statment in AXIsolatedObject::setProperty must be updated as well. using AXPropertyValueVariant = std::variant<std::nullptr_t, AXID, String, bool, int, unsigned, double, float, uint64_t, AccessibilityButtonState, Color, URL, LayoutRect, FloatPoint, FloatRect, IntPoint, IntRect, std::pair<unsigned, unsigned>, Vector<AccessibilityText>, Vector<AXID>, Vector<std::pair<AXID, AXID>>, Vector<String>, Path, OptionSet<AXAncestorFlag>, InsideLink, Vector<Vector<AXID>>, CharacterRange, std::pair<AXID, CharacterRange> #if PLATFORM(COCOA) @@ -262,6 +264,26 @@ struct AXPropertyChange { AXPropertyMap properties; // Changed properties. }; +struct NodeUpdateOptions { + AXPropertyNameSet properties; + bool shouldUpdateNode { false }; + bool shouldUpdateChildren { false }; + + NodeUpdateOptions(const AXPropertyNameSet& propertyNames, bool shouldUpdateNode, bool shouldUpdateChildren) + : properties(propertyNames) + , shouldUpdateNode(shouldUpdateNode) + , shouldUpdateChildren(shouldUpdateChildren) + { } + + NodeUpdateOptions(const AXPropertyNameSet& propertyNames) + : properties(propertyNames) + { } + + NodeUpdateOptions(AXPropertyName propertyName) + : properties({ propertyName }) + { } +}; + DECLARE_ALLOCATOR_WITH_HEAP_IDENTIFIER(AXIsolatedTree); class AXIsolatedTree : public ThreadSafeRefCountedAndCanMakeThreadSafeWeakPtr<AXIsolatedTree> , public AXTreeStore<AXIsolatedTree> { @@ -336,6 +358,9 @@ public: AXTextMarkerRange selectedTextMarkerRange(); void setSelectedTextMarkerRange(AXTextMarkerRange&&); + void queueNodeUpdate(AXCoreObject&, const NodeUpdateOptions&); + void processQueuedNodeUpdates(); + private: AXIsolatedTree(AXObjectCache&); static void storeTree(AXObjectCache&, const Ref<AXIsolatedTree>&); @@ -435,6 +460,11 @@ private: Lock m_changeLogLock; AXTextMarkerRange m_selectedTextMarkerRange WTF_GUARDED_BY_LOCK(m_changeLogLock); + + // Types of node updates (queued) + ListHashSet<AXID> m_needsUpdateChildren; + ListHashSet<AXID> m_needsUpdateNode; + HashMap<AXID, Vector<AXPropertyName>> m_needsPropertyUpdates; AG: can't we use an AXPropertyNameSet instead of Vector<AXPropertyName>? }; inline AXObjectCache* AXIsolatedTree::axObjectCache() const
(In reply to Andres Gonzalez from comment #9) > (In reply to Joshua Hoffman from comment #8) > + if (options.shouldUpdateChildren && > !m_needsUpdateChildren.contains(object.objectID())) > + m_needsUpdateChildren.add(object.objectID()); > + > + if (options.shouldUpdateNode && > !m_needsUpdateNode.contains(object.objectID())) > + m_needsUpdateNode.add(object.objectID()); > > AG: Don't need to do && !m_needsUpdateNode.contains(object.objectID()) since > the add method handles correctly whether the entry exists or not. Updated this and the condition for updateChildren. > + for (AXID nodeID : m_needsUpdateNode) { > > AG: we use objectID not nodeID, although in other naming we mix them. My > preference is to reserve node naming for DOM Nodes and use object for AX > objects. Updated. > > + if (!m_unresolvedPendingAppends.contains(nodeID)) > + m_unresolvedPendingAppends.set(nodeID, > AttachWrapper::OnMainThread); > > AG: Can we attach the wrapper on the main thread? I think that if we are > replacing an existing object, we cannot. Good catch! Yeah I will swap that to OnAXThread. > + // Types of node updates (queued) > + ListHashSet<AXID> m_needsUpdateChildren; > + ListHashSet<AXID> m_needsUpdateNode; > + HashMap<AXID, Vector<AXPropertyName>> m_needsPropertyUpdates; > > AG: can't we use an AXPropertyNameSet instead of Vector<AXPropertyName>? Yes, definitely. I just need to modify the parameter type of AXIsolatedTree::updateNodeProperties but that will be much cleaner.
Created attachment 468909 [details] Patch
Created attachment 468913 [details] Patch
Comment on attachment 468913 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=468913&action=review > Source/WebCore/accessibility/AXObjectCache.cpp:237 > +static const Seconds accessibilityUpdateTreeSnapshotTimerInterval { 100_ms }; "accessibility" feels unnecessary considering this is a static symbol inside an accessibility class. > Source/WebCore/accessibility/AXObjectCache.cpp:4330 > +void AXObjectCache::startSnapshotTimer() I think it's least confusing when these methods are named "start" and then the name of the timer. So "startUpdateTreeSnapshotTimer". > Source/WebCore/accessibility/AXObjectCache.cpp:4936 > + if (!accessibilityEnabled()) > + return; Is this necessary? What scenario are you trying to catch? > Source/WebCore/accessibility/AXObjectCache.cpp:4938 > + m_updateTreeSnapshotTimer.stop(); We probably want to unconditionally stop this timer. > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:998 > + queueNodeUpdate(*correspondingControl, { { }, true, false }); We've lost a lot of information at the callsite here. Before, we knew this object was going to get `updateNode` (fully recreated). Now, it's just curly braces and some un-named bools, which makes things less clear. Maybe we could make a static method in NodeUpdateOptions that constructs this for us with a more readable name? e.g. NodeUpdateOptions::nodeUpdate()? Same for the other updateNode callsites, and updateChildren callsites. > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:1279 > + auto* cache = axObjectCache(); This should be a WeakPtr rather than a raw pointer. > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:1284 > + if (auto* axObject = cache->objectForID(nodeID)) We should probably protect this object with a RefPtr instead of using a raw pointer. updateChildren does a lot of complicated logic that could cause this object to be destroyed (unless it's somehow protected elsewhere that you know about?) > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:1291 > + if (!m_unresolvedPendingAppends.contains(objectID)) > + m_unresolvedPendingAppends.set(objectID, AttachWrapper::OnAXThread); I think using ensure will require one less hash operation: m_unresolvedPendingAppends.ensure(axObject.objectID(), [] { return AttachWrapper::OnAXThread; }); > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:1299 > + if (auto* axObject = cache->objectForID(propertyUpdate.key)) Probably want to protect this with a RefPtr too. > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h:464 > + // Types of node updates (queued) Can we make this comment more descriptive? e.g. "Queued updates used for building a new tree snapshot." > Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:332 > +#if ENABLE(ACCESSIBILITY_ISOLATED_TREE) > + // Process all pending node changes before sending notification. > + if (auto tree = AXIsolatedTree::treeForPageID(m_pageID)) > + tree->processQueuedNodeUpdates(); > +#endif Can this extracted into a separate method since we repeat it a few times?
Comment on attachment 468913 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=468913&action=review >> Source/WebCore/accessibility/AXObjectCache.cpp:237 >> +static const Seconds accessibilityUpdateTreeSnapshotTimerInterval { 100_ms }; > > "accessibility" feels unnecessary considering this is a static symbol inside an accessibility class. Gotcha, will remove that. >> Source/WebCore/accessibility/AXObjectCache.cpp:4330 >> +void AXObjectCache::startSnapshotTimer() > > I think it's least confusing when these methods are named "start" and then the name of the timer. So "startUpdateTreeSnapshotTimer". Sounds good! >> Source/WebCore/accessibility/AXObjectCache.cpp:4936 >> + return; > > Is this necessary? What scenario are you trying to catch? We had debated this when adding selectedTextRangeTimerFired too, and I think we decided to leave this check in in case a user turns off an AT. But, I think the timer is so short that it is safe to remove. >> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:998 >> + queueNodeUpdate(*correspondingControl, { { }, true, false }); > > We've lost a lot of information at the callsite here. Before, we knew this object was going to get `updateNode` (fully recreated). Now, it's just curly braces and some un-named bools, which makes things less clear. Maybe we could make a static method in NodeUpdateOptions that constructs this for us with a more readable name? e.g. NodeUpdateOptions::nodeUpdate()? Same for the other updateNode callsites, and updateChildren callsites. I like that idea! Definitely makes things more readable. >> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:1284 >> + if (auto* axObject = cache->objectForID(nodeID)) > > We should probably protect this object with a RefPtr instead of using a raw pointer. updateChildren does a lot of complicated logic that could cause this object to be destroyed (unless it's somehow protected elsewhere that you know about?) No, I don't think it is protected anywhere else, so we should make this a RefPtr here. >> Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:332 >> +#endif > > Can this extracted into a separate method since we repeat it a few times? Yep!
Created attachment 468929 [details] Patch
(In reply to Joshua Hoffman from comment #15) > Created attachment 468929 [details] > Patch diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp index 890a0ec4857c..bb6c9aa21fa0 100644 --- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp +++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp @@ -523,7 +523,7 @@ void AXIsolatedTree::updateNode(AccessibilityObject& axObject) } } -void AXIsolatedTree::updatePropertiesForSelfAndDescendants(AccessibilityObject& axObject, const Vector<AXPropertyName>& properties) +void AXIsolatedTree::updatePropertiesForSelfAndDescendants(AccessibilityObject& axObject, const AXPropertyNameSet& properties) { AXTRACE("AXIsolatedTree::updatePropertiesForSelfAndDescendants"_s); ASSERT(isMainThread()); @@ -531,15 +531,19 @@ void AXIsolatedTree::updatePropertiesForSelfAndDescendants(AccessibilityObject& if (isUpdatingSubtree()) return; - Accessibility::enumerateDescendants<AXCoreObject>(axObject, true, [&properties, this] (auto& descendant) { - updateNodeProperties(descendant, properties); + AXPropertyNameSet propertySet; + for (const auto& property : properties) + propertySet.add(property); + + Accessibility::enumerateDescendants<AXCoreObject>(axObject, true, [&propertySet, this] (auto& descendant) { + queueNodeUpdate(descendant, { propertySet }); }); } -void AXIsolatedTree::updateNodeProperties(AXCoreObject& axObject, const Vector<AXPropertyName>& properties) +void AXIsolatedTree::updateNodeProperties(AXCoreObject& axObject, const AXPropertyNameSet& properties) { AXTRACE("AXIsolatedTree::updateNodeProperties"_s); - AXLOG(makeString("Updating properties ", properties, " for objectID ", axObject.objectID().loggingString())); + AXLOG(makeString("Updating properties for objectID ", axObject.objectID().loggingString(), ": ")); AG: what is the ": " for? AG: if you want to continue logging the PropertyNameSet, you probably need to write an operator << for it. But not necessary in this patch. You can get rid of this AXLOG all together or log the objectID in the AXTRACE. ASSERT(isMainThread()); if (isUpdatingSubtree()) @@ -547,6 +551,7 @@ void AXIsolatedTree::updateNodeProperties(AXCoreObject& axObject, const Vector<A AXPropertyMap propertyMap; for (const auto& property : properties) { + AXLOG(makeString("Property: ", property)); AG: ah I see that you are logging the property down here. It would be good to write the operator << and the AXLogger::log for it, but again not needed in this patch. switch (property) { case AXPropertyName::AccessKey: propertyMap.set(AXPropertyName::AccessKey, axObject.accessKey().isolatedCopy()); @@ -733,7 +738,7 @@ void AXIsolatedTree::updateNodeAndDependentProperties(AccessibilityObject& axObj bool updateTableAncestorColumns = is<AccessibilityTableRow>(axObject); for (auto* ancestor = axObject.parentObject(); ancestor; ancestor = ancestor->parentObject()) { if (ancestor->isTree()) { - updateNodeProperty(*ancestor, AXPropertyName::ARIATreeRows); + queueNodeUpdate(*ancestor, { AXPropertyName::ARIATreeRows }); if (!updateTableAncestorColumns) break; } @@ -990,7 +995,7 @@ void AXIsolatedTree::labelCreated(AccessibilityObject& axObject) { ASSERT(axObject.isLabel()); if (RefPtr correspondingControl = axObject.isLabel() ? axObject.correspondingControlForLabelElement() : nullptr) - updateNode(*correspondingControl); + queueNodeUpdate(*correspondingControl, NodeUpdateOptions::nodeUpdate()); } void AXIsolatedTree::updateLoadingProgress(double newProgressValue) @@ -1238,6 +1243,70 @@ AXTreePtr findAXTree(Function<bool(AXTreePtr)>&& match) #endif } +void AXIsolatedTree::queueNodeUpdate(AXCoreObject& object, const NodeUpdateOptions& options) +{ + ASSERT(isMainThread()); + + if (!options.shouldUpdateNode && options.properties.size()) { + // If we are updating a property, but our node is already in m_needsUpdateNode, don't queue anything. + if (m_needsUpdateNode.contains(object.objectID())) + return; + + AXPropertyNameSet properties; + if (m_needsPropertyUpdates.contains(object.objectID())) + properties = m_needsPropertyUpdates.get(object.objectID()); + + for (const auto& property : options.properties) + properties.add(property); + + m_needsPropertyUpdates.set(object.objectID(), WTFMove(properties)); + } AG: I think you can do just something like: if (m_needsPropertyUpdates.contains(object.objectID())) m_needsPropertyUpdates.get(object.objectID()).value.formUnion(options.properties); else m_needsPropertyUpdates.set(object.objectID(), options.properties); + + if (options.shouldUpdateChildren) + m_needsUpdateChildren.add(object.objectID()); + + if (options.shouldUpdateNode) + m_needsUpdateNode.add(object.objectID()); + + if (auto* cache = axObjectCache()) + cache->startUpdateTreeSnapshotTimer(); +} + +void AXIsolatedTree::processQueuedNodeUpdates() +{ + ASSERT(isMainThread()); + + WeakPtr cache = axObjectCache(); + if (!cache) + return; + + for (AXID nodeID : m_needsUpdateChildren) { + if (RefPtr axObject = cache->objectForID(nodeID)) AG: shouldn't we null check cache before using it? It may go away at any time I think. + updateChildren(*axObject, ResolveNodeChanges::No); + } + m_needsUpdateChildren.clear(); + + for (AXID objectID : m_needsUpdateNode) { + if (!m_unresolvedPendingAppends.contains(objectID)) { + m_unresolvedPendingAppends.ensure(objectID, [] { + return AttachWrapper::OnAXThread; + }); + } + } + m_needsUpdateNode.clear(); + + for (const auto& propertyUpdate : m_needsPropertyUpdates) { + if (m_unresolvedPendingAppends.contains(propertyUpdate.key)) + break; AG: shouldn't be continue to the next object? + + if (RefPtr axObject = cache->objectForID(propertyUpdate.key)) AG: shouldn't we null check cache? + updateNodeProperties(*axObject, propertyUpdate.value); + } + m_needsPropertyUpdates.clear(); + + queueRemovalsAndUnresolvedChanges({ }); +} + } // namespace WebCore #endif // ENABLE(ACCESSIBILITY_ISOLATED_TREE) diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h index c9609b8c8b1d..88d36893ae0b 100644 --- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h +++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h @@ -246,6 +246,8 @@ enum class AXPropertyName : uint16_t { VisibleRows, }; +using AXPropertyNameSet = HashSet<AXPropertyName, IntHash<AXPropertyName>, WTF::StrongEnumHashTraits<AXPropertyName>>; + // If this type is modified, the switchOn statment in AXIsolatedObject::setProperty must be updated as well. using AXPropertyValueVariant = std::variant<std::nullptr_t, AXID, String, bool, int, unsigned, double, float, uint64_t, AccessibilityButtonState, Color, URL, LayoutRect, FloatPoint, FloatRect, IntPoint, IntRect, std::pair<unsigned, unsigned>, Vector<AccessibilityText>, Vector<AXID>, Vector<std::pair<AXID, AXID>>, Vector<String>, Path, OptionSet<AXAncestorFlag>, InsideLink, Vector<Vector<AXID>>, CharacterRange, std::pair<AXID, CharacterRange> #if PLATFORM(COCOA) @@ -262,6 +264,36 @@ struct AXPropertyChange { AXPropertyMap properties; // Changed properties. }; +struct NodeUpdateOptions { + AXPropertyNameSet properties; + bool shouldUpdateNode { false }; + bool shouldUpdateChildren { false }; + + NodeUpdateOptions(const AXPropertyNameSet& propertyNames, bool shouldUpdateNode, bool shouldUpdateChildren) + : properties(propertyNames) + , shouldUpdateNode(shouldUpdateNode) + , shouldUpdateChildren(shouldUpdateChildren) + { } + + NodeUpdateOptions(const AXPropertyNameSet& propertyNames) + : properties(propertyNames) + { } + + NodeUpdateOptions(AXPropertyName propertyName) + : properties({ propertyName }) + { } + + static NodeUpdateOptions nodeUpdate() + { + return { { }, true, false }; + } + + static NodeUpdateOptions childrenUpdate() + { + return { { }, false, true }; + } AG: can these two be constexpr ? +}; + DECLARE_ALLOCATOR_WITH_HEAP_IDENTIFIER(AXIsolatedTree); class AXIsolatedTree : public ThreadSafeRefCountedAndCanMakeThreadSafeWeakPtr<AXIsolatedTree> , public AXTreeStore<AXIsolatedTree> { @@ -298,9 +330,9 @@ public: void updateChildren(AccessibilityObject&, ResolveNodeChanges = ResolveNodeChanges::Yes); void updateChildrenForObjects(const ListHashSet<RefPtr<AccessibilityObject>>&); void updateNodeProperty(AXCoreObject& object, AXPropertyName property) { updateNodeProperties(object, { property }); }; - void updateNodeProperties(AXCoreObject&, const Vector<AXPropertyName>&); + void updateNodeProperties(AXCoreObject&, const AXPropertyNameSet&); void updateNodeAndDependentProperties(AccessibilityObject&); - void updatePropertiesForSelfAndDescendants(AccessibilityObject&, const Vector<AXPropertyName>&); + void updatePropertiesForSelfAndDescendants(AccessibilityObject&, const AXPropertyNameSet&); void updateFrame(AXID, IntRect&&); void overrideNodeProperties(AXID, AXPropertyMap&&); @@ -336,6 +368,9 @@ public: AXTextMarkerRange selectedTextMarkerRange(); void setSelectedTextMarkerRange(AXTextMarkerRange&&); + void queueNodeUpdate(AXCoreObject&, const NodeUpdateOptions&); + void processQueuedNodeUpdates(); + private: AXIsolatedTree(AXObjectCache&); static void storeTree(AXObjectCache&, const Ref<AXIsolatedTree>&); @@ -435,6 +470,11 @@ private: Lock m_changeLogLock; AXTextMarkerRange m_selectedTextMarkerRange WTF_GUARDED_BY_LOCK(m_changeLogLock); + + // Queued node updates used for building a new tree snapshot. + ListHashSet<AXID> m_needsUpdateChildren; + ListHashSet<AXID> m_needsUpdateNode; + HashMap<AXID, AXPropertyNameSet> m_needsPropertyUpdates; }; inline AXObjectCache* AXIsolatedTree::axObjectCache() const
(In reply to Andres Gonzalez from comment #16) > (In reply to Joshua Hoffman from comment #15) > > Created attachment 468929 [details] > > Patch > > AXPropertyMap propertyMap; > for (const auto& property : properties) { > + AXLOG(makeString("Property: ", property)); > > AG: ah I see that you are logging the property down here. It would be good > to write the operator << and the AXLogger::log for it, but again not needed > in this patch. Sounds good! This is definitely something I'd love to clean up in the future. > + if (!options.shouldUpdateNode && options.properties.size()) { > + // If we are updating a property, but our node is already in > m_needsUpdateNode, don't queue anything. > + if (m_needsUpdateNode.contains(object.objectID())) > + return; > + > + AXPropertyNameSet properties; > + if (m_needsPropertyUpdates.contains(object.objectID())) > + properties = m_needsPropertyUpdates.get(object.objectID()); > + > + for (const auto& property : options.properties) > + properties.add(property); > + > + m_needsPropertyUpdates.set(object.objectID(), WTFMove(properties)); > + } > > AG: I think you can do just something like: I like that idea, that should work. > + > +void AXIsolatedTree::processQueuedNodeUpdates() > +{ > + ASSERT(isMainThread()); > + > + WeakPtr cache = axObjectCache(); > + if (!cache) > + return; > + > + for (AXID nodeID : m_needsUpdateChildren) { > + if (RefPtr axObject = cache->objectForID(nodeID)) > > AG: shouldn't we null check cache before using it? It may go away at any > time I think. Is the null check at the top not sufficient in this case? There are other places we null check the cache once and then use it unconditionally. > > + updateChildren(*axObject, ResolveNodeChanges::No); > + } > + m_needsUpdateChildren.clear(); > + > + for (AXID objectID : m_needsUpdateNode) { > + if (!m_unresolvedPendingAppends.contains(objectID)) { > + m_unresolvedPendingAppends.ensure(objectID, [] { > + return AttachWrapper::OnAXThread; > + }); > + } > + } > + m_needsUpdateNode.clear(); > + > + for (const auto& propertyUpdate : m_needsPropertyUpdates) { > + if (m_unresolvedPendingAppends.contains(propertyUpdate.key)) > + break; > > AG: shouldn't be continue to the next object? Ah yes—good catch! > + static NodeUpdateOptions nodeUpdate() > + { > + return { { }, true, false }; > + } > + > + static NodeUpdateOptions childrenUpdate() > + { > + return { { }, false, true }; > + } > > AG: can these two be constexpr ? Yes I'll update this.
(In reply to Joshua Hoffman from comment #17) > (In reply to Andres Gonzalez from comment #16) > > (In reply to Joshua Hoffman from comment #15) > > > + static NodeUpdateOptions nodeUpdate() > > + { > > + return { { }, true, false }; > > + } > > + > > + static NodeUpdateOptions childrenUpdate() > > + { > > + return { { }, false, true }; > > + } > > > > AG: can these two be constexpr ? > > Yes I'll update this. Tried this out and these actually cannot be constexpr, since the properties field (AXPropertyNameSet) in this struct is not a literal.
Created attachment 468947 [details] Patch
(In reply to Andres Gonzalez from comment #16) > (In reply to Joshua Hoffman from comment #15) > > Created attachment 468929 [details] > > Patch > + > + AXPropertyNameSet properties; > + if (m_needsPropertyUpdates.contains(object.objectID())) > + properties = m_needsPropertyUpdates.get(object.objectID()); > + > + for (const auto& property : options.properties) > + properties.add(property); > + > + m_needsPropertyUpdates.set(object.objectID(), WTFMove(properties)); > + } > > AG: I think you can do just something like: > > if (m_needsPropertyUpdates.contains(object.objectID())) > > m_needsPropertyUpdates.get(object.objectID()).value.formUnion(options. > properties); > else > m_needsPropertyUpdates.set(object.objectID(), > options.properties); > I've been thinking about this a bit and I think we can make it even simpler without using any conditions: auto existingProperties = m_needsPropertyUpdates.get(object.objectID()); m_needsPropertyUpdates.set(object.objectID(), existingProperties.unionWith(options.properties)); ExistingProperties will be empty if it doesn't exist in the map, so we can union with options.properties regardless.
Created attachment 468967 [details] Patch
(In reply to Joshua Hoffman from comment #20) > (In reply to Andres Gonzalez from comment #16) > > (In reply to Joshua Hoffman from comment #15) > > > Created attachment 468929 [details] > > > Patch > > + > > + AXPropertyNameSet properties; > > + if (m_needsPropertyUpdates.contains(object.objectID())) > > + properties = m_needsPropertyUpdates.get(object.objectID()); > > + > > + for (const auto& property : options.properties) > > + properties.add(property); > > + > > + m_needsPropertyUpdates.set(object.objectID(), WTFMove(properties)); > > + } > > > > AG: I think you can do just something like: > > > > if (m_needsPropertyUpdates.contains(object.objectID())) > > > > m_needsPropertyUpdates.get(object.objectID()).value.formUnion(options. > > properties); > > else > > m_needsPropertyUpdates.set(object.objectID(), > > options.properties); > > > > I've been thinking about this a bit and I think we can make it even simpler > without using any conditions: > > auto existingProperties = m_needsPropertyUpdates.get(object.objectID()); > m_needsPropertyUpdates.set(object.objectID(), > existingProperties.unionWith(options.properties)); > > ExistingProperties will be empty if it doesn't exist in the map, so we can > union with options.properties regardless. Yes, something like that, although unionWith creates a new set while formUnion operates in place.
Created attachment 468978 [details] Patch
(In reply to Joshua Hoffman from comment #17) > (In reply to Andres Gonzalez from comment #16) > > (In reply to Joshua Hoffman from comment #15) > > > Created attachment 468929 [details] > > > Patch > > > > AXPropertyMap propertyMap; > > for (const auto& property : properties) { > > + AXLOG(makeString("Property: ", property)); > > > > AG: ah I see that you are logging the property down here. It would be good > > to write the operator << and the AXLogger::log for it, but again not needed > > in this patch. > > Sounds good! This is definitely something I'd love to clean up in the future. > > > + if (!options.shouldUpdateNode && options.properties.size()) { > > + // If we are updating a property, but our node is already in > > m_needsUpdateNode, don't queue anything. > > + if (m_needsUpdateNode.contains(object.objectID())) > > + return; > > + > > + AXPropertyNameSet properties; > > + if (m_needsPropertyUpdates.contains(object.objectID())) > > + properties = m_needsPropertyUpdates.get(object.objectID()); > > + > > + for (const auto& property : options.properties) > > + properties.add(property); > > + > > + m_needsPropertyUpdates.set(object.objectID(), WTFMove(properties)); > > + } > > > > AG: I think you can do just something like: > > I like that idea, that should work. > > > + > > +void AXIsolatedTree::processQueuedNodeUpdates() > > +{ > > + ASSERT(isMainThread()); > > + > > + WeakPtr cache = axObjectCache(); > > + if (!cache) > > + return; > > + > > + for (AXID nodeID : m_needsUpdateChildren) { > > + if (RefPtr axObject = cache->objectForID(nodeID)) > > > > AG: shouldn't we null check cache before using it? It may go away at any > > time I think. > > Is the null check at the top not sufficient in this case? There are other > places we null check the cache once and then use it unconditionally. > > AG: maybe we can get away with no checking that the cache is still alive as we iterate here, but it would be safer and not costly to check inside the loop. updateChildren is a complex operation that can trigger indirectly the destruction of the cache. The rest LGTM. Thanks! > > + updateChildren(*axObject, ResolveNodeChanges::No); > > + } > > + m_needsUpdateChildren.clear(); ...
Comment on attachment 468978 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=468978&action=review > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:1251 > + // If we are updating a property, but our node is already in m_needsUpdateNode, don't queue anything. It would be nice if this comment explained "why" rather than "what" (since the code tells us "what"). For example (re-word as desired, just an idea): // If we're going to recompute all properties for the node, indicated by presence in m_needsUpdateNode, don't bother queueing this individual property update. > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:1257 > + auto addResult = m_needsPropertyUpdates.add(object.objectID(), options.properties); > + > + if (!addResult.isNewEntry) nit: the newline between these statements feels unnecessary.
Created attachment 468983 [details] Patch
Thanks Tyler and Andres for your comments! They are all addressed in the latest update.
(In reply to Joshua Hoffman from comment #26) > Created attachment 468983 [details] > Patch + WeakPtr cache = axObjectCache(); + if (!cache) + return; + + for (AXID nodeID : m_needsUpdateChildren) { + if (!cache) + break; AG: return ? Or we need to exec the rest of the function ? + + if (RefPtr axObject = cache->objectForID(nodeID)) + updateChildren(*axObject, ResolveNodeChanges::No); + } + m_needsUpdateChildren.clear(); + + for (AXID objectID : m_needsUpdateNode) { + if (!m_unresolvedPendingAppends.contains(objectID)) { + m_unresolvedPendingAppends.ensure(objectID, [] { + return AttachWrapper::OnAXThread; + }); + } + } + m_needsUpdateNode.clear(); + + for (const auto& propertyUpdate : m_needsPropertyUpdates) { + if (m_unresolvedPendingAppends.contains(propertyUpdate.key)) + continue; + + if (!cache) + break; AG: same here. + + if (RefPtr axObject = cache->objectForID(propertyUpdate.key)) + updateNodeProperties(*axObject, propertyUpdate.value); + } + m_needsPropertyUpdates.clear(); + + queueRemovalsAndUnresolvedChanges({ }); +}
(In reply to Andres Gonzalez from comment #28) > (In reply to Joshua Hoffman from comment #26) > > Created attachment 468983 [details] > > Patch > > + WeakPtr cache = axObjectCache(); > + if (!cache) > + return; > + > + for (AXID nodeID : m_needsUpdateChildren) { > + if (!cache) > + break; > > AG: return ? Or we need to exec the rest of the function ? I think we’d still want to resolve appends, which we do at the end of this method.
cq-, let's make sure EWS is still OK
(In reply to Tyler Wilcock from comment #30) > cq-, let's make sure EWS is still OK I think it won't be landed if something fails, so it makes no difference whether you CQ+ now or later, unless there are more comments of course.
(In reply to Andres Gonzalez from comment #31) > (In reply to Tyler Wilcock from comment #30) > > cq-, let's make sure EWS is still OK > > I think it won't be landed if something fails, so it makes no difference > whether you CQ+ now or later, unless there are more comments of course. Commit queue only checks that Mac WK2 builds and passes layout tests, it doesn't wait for anything else as far as I know
Committed 271907@main (d39e84f7bea7): <https://commits.webkit.org/271907@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 468983 [details].