[webkit-reviews] review denied: [Bug 62289] ARIA live regions don't trigger notifications for elements that aren't in the AX tree : [Attachment 96437] patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied chris fleizach <cfleizach at apple.com>'s
request for review:
Bug 62289: ARIA live regions don't trigger notifications for elements that
aren't in the AX tree
https://bugs.webkit.org/show_bug.cgi?id=62289

Attachment 96437: patch
https://bugs.webkit.org/attachment.cgi?id=96437&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=96437&action=review

Looks almost right, but I had a few concerns. review- because we don’t want a
vector that might give us unwanted O(n^2) performance.

> Source/WebCore/accessibility/AXObjectCache.cpp:373
> +    size_t updateChildrenPos = m_childrenToUpdate.find(renderer);
> +    if (updateChildrenPos != WTF::notFound)
> +	   m_childrenToUpdate.remove(updateChildrenPos);

If we need to support efficient removal, this should be a HashSet or
ListHashSet. It’s not good to search in a vector.

> Source/WebCore/accessibility/AXObjectCache.cpp:446
> +    m_childrenUpdateTimer.stop();

It’s usually not necessary to stop the timer. Why was this needed?

> Source/WebCore/accessibility/AXObjectCache.cpp:452
> +    unsigned i = 0, count = m_childrenToUpdate.size();
> +    for (i = 0; i < count; ++i) {
> +	   if (AccessibilityObject* axObj = getOrCreate(m_childrenToUpdate[i]))

> +	      
axObj->childrenChanged(AccessibilityObject::CreateParentObjects);
> +    }

This initializes i twice. It should use size_t. And normally for a local
variable name we’d use “size” rather than mixing “count” and “size”.

The name axObj is not so good. I would just name the local “object”.

> Source/WebCore/accessibility/AXObjectCache.cpp:454
> +    m_childrenToUpdate.clear();

It’s safer to make a copy of the vector to iterate it. If there is any chance
that childrenChanged could add items to the m_childrenToUpdate collection
during the iteration, then we need protection.

> Source/WebCore/accessibility/AXObjectCache.cpp:467
> +	   size_t updateChildrenPos = m_childrenToUpdate.find(renderer);

Again, if we are doing “find” we should probably be using a set rather than a
vector. If ordering matters, it can be a ListHashSet.

> Source/WebCore/accessibility/AXObjectCache.cpp:475
> +	   if (AccessibilityObject* obj = m_objects.get(axID).get())
> +	      
obj->childrenChanged(AccessibilityObject::DoNotCreateParentObjects);

You should name this “object” instead of “obj”.