Create ancestry of isolated objects instead of generating the entire subtree for an ancestor.
<rdar://problem/87803473>
Created attachment 449541 [details] Patch
Created attachment 449617 [details] Patch
Comment on attachment 449617 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=449617&action=review > Source/WebCore/ChangeLog:8 > + No new tests (OOPS!). I think commit queue will fail if this line is still in the patch. > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:230 > +void AXIsolatedTree::addChanges(const NodeChange& nodeChange, Vector<AXID>&& childrenIDs) Would naming this something like `queueChanges` be more informative of what this method is doing? > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:348 > + auto* axParent = ancestor.parentObject(); parentObject() includes ignored objects (vs. parentObjectUnignored()). Is that OK? It seems our implementation of AXIsolatedObject::parentObjectUnignored assumes the parent is unignored, so adding an ignored object to the tree might break that assumption? AXCoreObject* AXIsolatedObject::parentObjectUnignored() const { return tree()->nodeForID(parent()).get(); }
Comment on attachment 449617 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=449617&action=review > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:205 > +AXIsolatedTree::NodeChange AXIsolatedTree::createNodeChange(AXCoreObject& axObject, AXID parentID, bool attachWrapper) What does this give us by removing the Ref on the returned object? the word create, at least at apple, usually implies a new object is allocated that is retained. wonder if this name should be different
Created attachment 449675 [details] Patch
(In reply to Tyler Wilcock from comment #5) > Comment on attachment 449617 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=449617&action=review > > > Source/WebCore/ChangeLog:8 > > + No new tests (OOPS!). > > I think commit queue will fail if this line is still in the patch. That was strange, it was correct in my local copy, so I must have uploaded the patch without saving the ChangeLog. > > > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:230 > > +void AXIsolatedTree::addChanges(const NodeChange& nodeChange, Vector<AXID>&& childrenIDs) > > Would naming this something like `queueChanges` be more informative of what > this method is doing? Yes! thanks, done. > > > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:348 > > + auto* axParent = ancestor.parentObject(); > > parentObject() includes ignored objects (vs. parentObjectUnignored()). Is > that OK? It seems our implementation of > AXIsolatedObject::parentObjectUnignored assumes the parent is unignored, so > adding an ignored object to the tree might break that assumption? > > AXCoreObject* AXIsolatedObject::parentObjectUnignored() const > { > return tree()->nodeForID(parent()).get(); > } We may need to revisit this since that could create inconsistencies in the isolated Tre because the parent will not have this object as a child.
(In reply to chris fleizach from comment #6) > Comment on attachment 449617 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=449617&action=review > > > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:205 > > +AXIsolatedTree::NodeChange AXIsolatedTree::createNodeChange(AXCoreObject& axObject, AXID parentID, bool attachWrapper) > > What does this give us by removing the Ref on the returned object? > the word create, at least at apple, usually implies a new object is > allocated that is retained. wonder if this name should be different We are not removing the Ref to the object but storing it in the NodeChange. I renamed it to nodeChangeForObject.
Committed r288405 (246296@main): <https://commits.webkit.org/246296@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 449675 [details].