Fix for accessibility/mac/aria-expanded-notifications.html in isolated tree mode.
Created attachment 406790 [details] Patch
Comment on attachment 406790 [details] 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?
Created attachment 406801 [details] Patch
(In reply to Darin Adler from comment #2) > Comment on attachment 406790 [details] > 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". Yes! fixed. > > > 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. The original comment referred to parentObject since they were using a while loop to traverse the hierarchy upwards instead of findAncestor. I have not confirm that parentObject indeed may cause the object to be deleted, so I trusted whoever wrote that. But the point is kind of mute now that we are using the RefPtr. > > > 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". Fixed. > > > 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? Yes, fixed.
Committed r265842: <https://trac.webkit.org/changeset/265842> All reviewed patches have been landed. Closing bug and clearing flags on attachment 406801 [details].
<rdar://problem/67365749>