Code cleanup in preparation for refactoring of computing relationships between AXObjects.
<rdar://problem/92455544>
Created attachment 458523 [details] Patch
Comment on attachment 458523 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=458523&action=review Seems like a really nice improvement! > Source/WebCore/accessibility/AccessibilityObject.cpp:4018 > + if (!labelFor.size()) Could this be labelFor.isEmpty()?
Comment on attachment 458523 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=458523&action=review > Source/WebCore/accessibility/AccessibilityObject.cpp:3953 > +AXCoreObject::AccessibilityChildrenVector AccessibilityObject::activeDescendantOf() const this feels like it should be plural activeDescendantsOf > Source/WebCore/accessibility/AccessibilityObject.h:307 > + AccessibilityChildrenVector descriptionFor() const override; I don't feel like these methods indicate they're returning elements. descriptionFor() sounds like it could be a string returning function can we call it something like "descriptionElementsFor()" or "errorMessageElementsFor()"
Created attachment 458604 [details] Patch
(In reply to Tyler Wilcock from comment #3) > Comment on attachment 458523 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=458523&action=review > > Seems like a really nice improvement! > > > Source/WebCore/accessibility/AccessibilityObject.cpp:4018 > > + if (!labelFor.size()) > > Could this be labelFor.isEmpty()? Yes, using isEmpty() in more places where it makes the code more legible.
(In reply to chris fleizach from comment #4) > Comment on attachment 458523 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=458523&action=review > > > Source/WebCore/accessibility/AccessibilityObject.cpp:3953 > > +AXCoreObject::AccessibilityChildrenVector AccessibilityObject::activeDescendantOf() const > > this feels like it should be plural > activeDescendantsOf Renamed to activeDescendantOfObjects. ActiveDescendant is singular because it refers to this object, and the method returns the objects that "contain" this as their active descendant. Hopefully the naming now is a bit more clear. Let me know. > > > Source/WebCore/accessibility/AccessibilityObject.h:307 > > + AccessibilityChildrenVector descriptionFor() const override; > > I don't feel like these methods indicate they're returning elements. > descriptionFor() sounds like it could be a string returning function > > can we call it something like "descriptionElementsFor()" or > "errorMessageElementsFor()" Chose Objects as part of the name to avoid confusion with DOM Elements. Notice the symmetric methods for a given relationship. For instance, we have labelledByObjects and labelForObjects. Hopefully this is a bit more clear now. Let me know. Thanks.
Created attachment 458607 [details] Patch Fix for iOS build.
Comment on attachment 458607 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=458607&action=review > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1084 > + linkedUIElements = flowToObjects(); Will this overwrite existing elements in `linkedUIElements`? If so, should we concat these vectors instead? > Source/WebCore/accessibility/AccessibilityTableRow.cpp:154 > + if (ownedObjects.size()) { Would this be better as !ownedObjects.isEmpty()?
Created attachment 458618 [details] Patch
(In reply to Tyler Wilcock from comment #9) > Comment on attachment 458607 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=458607&action=review > > > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1084 > > + linkedUIElements = flowToObjects(); > > Will this overwrite existing elements in `linkedUIElements`? If so, should > we concat these vectors instead? It is never used to concat linked objects. So while at it, also removed the out parameter and renamed to linkedObjects() like all the other relations methods. > > > Source/WebCore/accessibility/AccessibilityTableRow.cpp:154 > > + if (ownedObjects.size()) { > > Would this be better as !ownedObjects.isEmpty()? In this case I think either one is fine, but I opted for the shortest with no negation.
Created attachment 458639 [details] Patch
Committed r293650 (250154@main): <https://commits.webkit.org/250154@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 458639 [details].