[Webkit-unassigned] [Bug 203408] Create base class common to AccessibilityObject and AXIsolatedTreeNode.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=203408

--- Comment #4 from Andres Gonzalez <andresg_22 at apple.com> ---
(In reply to chris fleizach from comment #2)
> Comment on attachment 381926 [details]
> Created AccessibilityObjectBase, a common base class for both
> AccessibilityObject and AXIsolatedTreeNode.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=381926&action=review
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:336
> >      for (const auto& child : axRenderImage->children()) {
> 
> the method signature

It is fine to return an AccessibilityObject here, no need to return the base class. Although we could change the method signature to return an AccessibilityImageMapLink. is that what you mean?

> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:2973
> > +    tree->setRoot(root);
> 
> why do we want to store the actual root instead of the rootID?

The root needs to be set during the creation of the tree because:

- (id)isolatedTreeRootObject
{
...
            auto tree = cache->generateIsolatedAccessibilityTree();
...
            if (auto rootNode = tree->rootNode())
                return rootNode->wrapper();
...
    return nil;
}

and tree->rootNode() will be nil because it won't find any element in the HashMap for the root ID. If this returns nil, everything else fails.

> 
> > Source/WebCore/accessibility/AccessibilityTableRow.cpp:147
> > +    return downcast<AccessibilityTableCell>(cell);
> 
> this downcast seems unnecessary since we're returning a
> AccessibilityObjectBase

Yes, fixed.

> 
> > Source/WebCore/accessibility/ios/AXObjectCacheIOS.mm:44
> > +    return wrapper;
> 
> if we're detaching a wrapper I don't think we should be returning it,
> because it's essentially deallocated when it comes back after this method

Not if the caller assigns it to a variable, strong reference. I did this to allow for swapping the underlying accessible object. For instance:

AXIsolatedTreeNode::f() {
    // cannot fulfill request, have to go back to main thread.
    AccessibilityObject* axObject = get corresponding live object from the ID;
    auto* wrapper = cache->detachWrapper(*this);
    axObject->attachWrapper(wrapper);
    // send to main thread axObject->f()
}

But I'm not using it yet in this patch. So I could revert that if you wish.

> 
> > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:121
> > +    m_readerThreadNodeMap.add(root->objectID(), WTFMove(root));
> 
> I'm concerned about moving the root into the reader thread here. I think
> that's already supposed to have happened

It hasn't happened on generating the tree when the root needs to be set. So that's why we now do setRoot instead of just setRootID, as explained above.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20191025/4785f950/attachment.htm>