[webkit-reviews] review granted: [Bug 215613] Fix for accessibility/mac/aria-expanded-notifications.html in isolated tree mode. : [Attachment 406790] 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 215613: Fix for accessibility/mac/aria-expanded-notifications.html in
isolated tree mode.
https://bugs.webkit.org/show_bug.cgi?id=215613

Attachment 406790: Patch

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




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

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

> Source/WebCore/accessibility/AXObjectCache.cpp:1655
> +    if (auto* object = get(node)) {

I’d like to see get return a RefPtr, then we don’t have to do this "protected"
thing. But that’s probably a big change.

Another way do it is to write this:

    if (auto object = makeRefPtr(node))

I like that better than using a separate "protectedObject".

> Source/WebCore/accessibility/AXObjectCache.cpp:1656
> +	   // This object might be deleted during the call to findAncestor,
thus increase its ref count to avoid destruction.

Are you are that’s true? Seems peculiar that findAncestor can do this. How
would finding an ancestor have side effects? We’re just calling roleValue. Does
that function do non-obvious work?

Seems more likely that postNotification could delete the object. So I guess we
probably do need to use RefPtr. But the comment is misleading, I think.

> Source/WebCore/accessibility/AXObjectCache.cpp:1660
> +	   auto* ancestor =
Accessibility::findAncestor<AccessibilityObject>(*object, false, [] (const
auto& obj) {

Please don’t use abbreviations like "obj" in WebKit code. I suggest "candidate"
here, but another word would be fine. Also can just be "auto&", no need to
state "const".

> Source/WebCore/accessibility/AXObjectCache.cpp:1670
> +	       switch (obj.roleValue()) {
> +	       case AccessibilityRole::Tree:
> +	       case AccessibilityRole::TreeGrid:
> +	       case AccessibilityRole::Grid:
> +	       case AccessibilityRole::Table:
> +	       case AccessibilityRole::Browser:
> +		   return true;
> +	       default:
> +		   return false;
> +	       }

Should we have a function called something like supportsRowCountChanged instead
of writing out the switch like this?