[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