Summary: | Create ancestry of isolated objects instead of generating the entire subtree for an ancestor. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Andres Gonzalez <andresg_22> | ||||||||
Component: | Accessibility | Assignee: | Andres Gonzalez <andresg_22> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | aboxhall, andresg_22, apinheiro, cfleizach, dmazzoni, ews-watchlist, jcraig, jdiggs, samuel_white, tyler_w, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Andres Gonzalez
2022-01-19 18:20:47 PST
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]. |