[webkit-reviews] review requested: [Bug 193348] AX: Introduce isolated accessibility tree : [Attachment 360161] patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has asked	for review:
Bug 193348: AX: Introduce isolated accessibility tree
https://bugs.webkit.org/show_bug.cgi?id=193348

Attachment 360161: patch

https://bugs.webkit.org/attachment.cgi?id=360161&action=review




--- Comment #48 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 360161
  --> https://bugs.webkit.org/attachment.cgi?id=360161
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=360161&action=review

> Source/WebCore/accessibility/AXObjectCache.cpp:2919
> +Ref<AXIsolatedTreeNode>
AXObjectCache::createIsolatedAccessibilityTree(AccessibilityObject& object,
AXID parentID, AXIsolatedTree& tree, Vector<RefPtr<AXIsolatedTreeNode>>& nodes)

Should that be Vector<RefPtr<AXIsolatedTreeNode>>&& if ownership is being
transferred.

I'm confused about this function. It's called createIsolatedAccessibilityTree()
but it takes a tree, rather than returning a tree. Is it really appending nodes
to the tree? Maybe it should have "recursive" in the name too?

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:37
> +static unsigned s_treeIDCount = 0;

Is this a count of IDs? Maybe call it currentTreeID? Would be cleaner to wrap
this in a function that provides unique IDs.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:63
> +RefPtr<AXIsolatedTree> AXIsolatedTree::treeForPageID(uint64_t pageID, bool
create)

Do you really want a tree per Page? Page outlives navigations, so do you swap
out the tree on navigation? What's the lifetime of trees which aren't for the
current document?

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:85
> +    ASSERT(!isMainThread());

Can you assert that this is the reader thread?

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h:49
> +    WEBCORE_EXPORT static RefPtr<AXIsolatedTree> treeForPageID(uint64_t,
bool create);

Name uint64_t here.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.h:63
> +    bool isRootNode() const { return m_isRootNode; }

Can you tell that it's the root node because it has no parent?

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.h:78
> +    const Vector<AXID>& children() { return m_children; };

Function should be const.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.h:115
> +    AXID m_parent;
> +    AXID m_identifier;
> +    bool m_isRootNode;
> +    bool m_initialized;
> +    Vector<AXID> m_children;
> +
> +#if PLATFORM(COCOA)
> +    RetainPtr<WebAccessibilityObjectWrapper> m_wrapper;
> +#endif
> +
> +    HashMap<AXPropertyName, AttributeValueVariant,
WTF::IntHash<AXPropertyName>, WTF::StrongEnumHashTraits<AXPropertyName>>
m_attributeMap;

You can probably pack this better. Use dump-class-layout to optimize.