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

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied 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 359768: patch

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




--- Comment #32 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 359768
  --> https://bugs.webkit.org/attachment.cgi?id=359768
patch

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

> Source/WebCore/ChangeLog:9
> +	   In order to improve performance when requesting the accessibility
hierarchy, we introduce the idea of a "static accessibility tree" which could
be accessed on a different thread by assistive technologies.

Can we wrap lines at some point earlier?
It's super long. Also rename static -> isolated?

> Source/WebCore/ChangeLog:10
> +	   That is accomplished by storing all the data needed to answer
accessibility attribute queries in a static object that mirrors the "live"
AccessibilityObjects (which interact with both DOM and Render trees).

Ditto.

> Source/WebCore/ChangeLog:11
> +	   These static objects are generally created after layout is done and
final tasks are being performed. They are then stored in the static tree
representation and able to be read from anywhere.

Ditto.

> Source/WebCore/ChangeLog:12
> +	   Tactically this is done with AXIsolatedTreeNodes inside of an
AXIsolatedTreeCache. The TreeNodes implement an AccessibilityObjectInterface
shared with AccessibilityObject

Ditto.

> Source/WebCore/accessibility/AXObjectCache.cpp:2916
> +RefPtr<AXIsolatedTreeNode>
AXObjectCache::createStaticAccessibilityTree(AccessibilityObject& object, AXID
parentID, AXIsolatedTreeCache& cache, Vector<RefPtr<AXIsolatedTreeNode>>&
nodes)

Make this return Ref.

> Source/WebCore/accessibility/AXObjectCache.cpp:2940
> +    RefPtr<AXIsolatedTreeCache> cache =
AXIsolatedTreeCache::cacheForPage(m_document.page());

Use auto.

> Source/WebCore/accessibility/AXObjectCache.cpp:2942
> +    RefPtr<AXIsolatedTreeNode> root =
createStaticAccessibilityTree(*rootObject(), 0, *cache, nodes);

Ditto. Hm... can we define InvalidAXID or something instead of hard-coding the
magic number of 0?

> Source/WebCore/accessibility/AXObjectCache.cpp:2944
> +    

Nit: whitespaces.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeCache.cpp:38
> +HashMap<Page*, RefPtr<AXIsolatedTreeCache>>&
AXIsolatedTreeCache::pageRootMap()

I think this map should be a static local variable in
AXIsolatedTreeCache::cacheForPage
since any caller of this function would have to grab a lock.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeCache.cpp:69
> +    RefPtr<AXIsolatedTreeCache> cache = pageRootMap().get(page);

This code needs a lock since this function can run concurrently in multiple
threads.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeCache.h:48
> +    WEBCORE_EXPORT static RefPtr<AXIsolatedTreeCache> cacheForPage(Page*);

This function should return Ref since it never returns nullptr.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeCache.h:66
> +    static HashMap<Page*, RefPtr<AXIsolatedTreeCache>>& pageRootMap();

Since we never insert a null AXIsolatedTreeCache into this map, it should be
Ref<AXIsolatedTreeCache>.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeCache.h:67
> +    static HashMap<AXIsolatedCacheID, RefPtr<AXIsolatedTreeCache>>&
treeCache();

Ditto.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeCache.h:70
> +    HashMap<AXID, RefPtr<AXIsolatedTreeNode>> m_cache;

Ditto. Use Ref<AXIsolatedTreeNode>.
Why don't we call this m_readerThreadNodeMap to make the intent clear without a
comment?
Also, there is no need to say this is the cache since the entire class is
called AXIsolatedTree*Cache*.
But it's almost misleading that this class is called AXIsolatedTree*Cache*
since AT exclusively accesses the accessibility tree via this "cache".
I think I would prefer calling this simply AXIsolatedTree altogether.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeCache.h:73
> +    Vector<RefPtr<AXIsolatedTreeNode>> m_appendChangeLog;

Ditto. Also, why don't we call this something like m_pendingAppends to convey
the semantics that these changes haven't taken place yet.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeCache.h:74
> +    Vector<AXID> m_removeChangeLog;

Ditto.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.h:127
> +    

Nit: Whitespace.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.h:131
> +    

Ditto.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.h:134
> +    

Ditto.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.h:140
> +    

Ditto.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.h:144
> +    

Ditto.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.h:148
> +    

Ditto.

> Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectBase.mm:53
> +    WebCore::Page* page = m_page->corePage();

This code is definitely not safe to run in non-main thread.
We can't access WebKit::WebPage in non-main thread.
r- because of this thread safety issue.
One way to make this code more thread safe is to key the mapping based on
PageID.

So when WKAccessibilityWebPageObjectBase is created in the main thread, it
could store its PageID

> Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectBase.mm:59
> +    

Nit: Whitespace.

> Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectBase.mm:75
>      WebCore::Page* page = m_page->corePage();

It's not safe to access WebCore::Page object in a separate thread like this
because page can be deleted meanwhile.

> Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectBase.mm:79
> +    AXObjectCache* cache = [self axObjectCache];

I think this whole logic to get WebCore::Page & accessing the map should be
moved to the dispatchBlock below.

> Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectBase.mm:92
> +	       dispatch_sync(dispatch_get_main_queue(), dispatchBlock);

Do we really need to dispatch sync?
Can we, for example, dispatch_async then return nil for now until the tree
becomes available?
We can keep it this way if an alternative is hard to implement. I was just
wondering.

> Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectBase.mm:95
> +    auto isolatedCache = AXIsolatedTreeCache::cacheForPage(page);

This code is definitely not safe to run in non-main thread.
AXIsolatedTreeCache::cacheForPage accesses HashMap which is mutated in the main
thread without a lock.
r- because of this thread safety issue. We need a lock in cacheForPage.

> Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectBase.mm:96
> +    if (!isolatedCache) {

It appears to me that this would never be true since we never return nullptr in
cacheForPage.

> Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectBase.mm:105
> +    

Nit: whitespace.