[webkit-reviews] review granted: [Bug 213435] AXIsolatedTree::generateSubtree should properly assign the generated subtree to its parent node. : [Attachment 402400] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Andres Gonzalez
<andresg_22 at apple.com>'s request for review:
Bug 213435: AXIsolatedTree::generateSubtree should properly assign the
generated subtree to its parent node.
https://bugs.webkit.org/show_bug.cgi?id=213435

Attachment 402400: Patch

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




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 402400
  --> https://bugs.webkit.org/attachment.cgi?id=402400
Patch

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

review+ but I don’t understand the correctness of the isLocked/LockHolder use.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:164
> +void AXIsolatedTree::updateChildrenIDs(AXID axID, Vector<AXID>&&
childrenIDs)

Seems like we missed an opportunity to move the vector in one of the places
below that do it. The make_pair could use WTFMove, I guess. Without that, the
function just copies the vector twice.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:174
> +    if (m_changeLogLock.isLocked())
> +	   m_pendingChildrenUpdates.append(std::make_pair(axID, childrenIDs));
> +    else {
> +	   LockHolder locker { m_changeLogLock };
> +	   m_pendingChildrenUpdates.append(std::make_pair(axID, childrenIDs));
> +    }

Is this really a safe way to use a lock? I’m surprised it’s OK and not racy to
check isLocked like this.

Seems an inelegant idiom; looks like an attempt to simulate recursive lock
semantics.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:379
> -    LockHolder locker { m_changeLogLock };
> -    m_pendingSubtreeRemovals.append(axID);
> +    {
> +	   LockHolder locker { m_changeLogLock };
> +	   m_pendingSubtreeRemovals.append(axID);
> +    }

This change is entirely stylistic, I guess? Doesn’t make any semantic change.