Bug 235389 – Create ancestry of isolated objects instead of generating the entire subtree for an ancestor.

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Andres Gonzalez]

Create ancestry of isolated objects instead of generating the entire subtree for an ancestor.

[Radar WebKit Bug Importer]

<rdar://problem/87803473>

[Andres Gonzalez]

Created attachment 449541 [details]
Patch

[Andres Gonzalez]

<rdar://problem/87803473>

[Andres Gonzalez]

Created attachment 449617 [details]
Patch

[Tyler Wilcock]

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();
}

[chris fleizach]

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

[Andres Gonzalez]

Created attachment 449675 [details]
Patch

[Andres Gonzalez]

(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.

[Andres Gonzalez]

(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.

[EWS]

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].