[webkit-reviews] review granted: [Bug 261968] AX: AXIsolatedObject::m_propertyMap is not sparse : [Attachment 467844] Patch

[bugzilla-daemon at webkit.org]

Tyler Wilcock <tyler_w at apple.com> has granted  review:
Bug 261968: AX: AXIsolatedObject::m_propertyMap is not sparse
https://bugs.webkit.org/show_bug.cgi?id=261968

Attachment 467844: Patch

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




--- Comment #9 from Tyler Wilcock <tyler_w at apple.com> ---
Comment on attachment 467844
  --> https://bugs.webkit.org/attachment.cgi?id=467844
Patch

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

Big improvement!

> Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp:55
> +    // Allocates a capacity based on the minimum of node properties stored
in the sparse map.

Minor nit — the term "sparse map" makes sense in the context of this patch, but
we don't explain that term anywhere, so it might be a bit confusing in the
future. Since our behavior will now inherently be to only insert "sparsely"
into the property map, maybe the comment should be something like:

// Allocate a capacity based on the minimum properties an object has (based on
measurements from a real webpage).

Re-word as you please if you have a better idea.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp:422
> +	       [](auto&) { return false; }

Might be nice to have an ASSERT_NOT_REACHED() here in case we forget to update
this after updating the variant. I also wonder if we should have a comment next
to the definition of AXPropertyValueVariant mentioning that if anything is
added or removed from the variant, we must update this function.