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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Jan 25 14:17:07 PST 2019


Ryosuke Niwa <rniwa at webkit.org> has granted chris fleizach
<cfleizach at apple.com>'s request 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 #47 from Ryosuke Niwa <rniwa at webkit.org> ---
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/isolatedtree/AXIsolatedTree.cpp:72
> +    if (auto tree = pageMap.get().get(pageID)) {
> +	   return makeRefPtr(tree);
> +    }

Nit: No curly braces around a single line statement.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:75
> +    if (!create)
> +	   return nullptr;

I think it's cleaner to have a separate function which creates the tree.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:76
> +    

Nit: whitespace.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:77
> +    RefPtr<AXIsolatedTree> newTree = AXIsolatedTree::create();

Use auto?

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.cpp:42
> +    initializeAttributeData(object);

Oh I mean that set m_initialized AFTER initializeAttributeData.
Otherwise, setProperty in initializeAttributeData would hit its assertion,
right?

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.cpp:63
> +    

Nit: whitespace.

> Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectBase.mm:103
> +    auto tree = AXIsolatedTree::treeForPageID(m_pageID);

We need to check null-ness of the tree here.

> Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectBase.mm:106
> +    auto rootNode = tree->rootNode();
> +    if (!rootNode)
> +	   generateBlock();

This check is now unnecessary because we'd always create the root node as we
create the tree.

> Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectBase.mm:108
> +    tree->applyPendingChanges();

Since it's only safe to invoke this function in the reader thread,
we might as well as assert that we're not in the main thread
instead of having a fallback for the main thread case.


More information about the webkit-reviews mailing list