Bug 253612

Summary: AX: Stop caching 9 isolated object properties unnecessarily
Product: WebKit Reporter: Tyler Wilcock <tyler_w>
Component: AccessibilityAssignee: Tyler Wilcock <tyler_w>
Status: RESOLVED FIXED    
Severity: Normal CC: aboxhall, andresg_22, apinheiro, cfleizach, dmazzoni, ews-watchlist, jcraig, jdiggs, samuel_white, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description Tyler Wilcock 2023-03-08 15:46:01 PST
We have many properties that can be expressed purely in terms of AXCoreObject methods.
Comment 1 Radar WebKit Bug Importer 2023-03-08 15:46:12 PST
<rdar://problem/106456887>
Comment 2 Tyler Wilcock 2023-03-08 15:52:15 PST
Created attachment 465365 [details]
Patch
Comment 3 chris fleizach 2023-03-08 16:03:54 PST
Comment on attachment 465365 [details]
Patch

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

did you measure any improvements from this one?

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:-1054
> -    AXCoreObject::AccessibilityChildrenVector tabs;

how come we don't need this?
Comment 4 Tyler Wilcock 2023-03-08 16:05:45 PST
(In reply to chris fleizach from comment #3)
> Comment on attachment 465365 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=465365&action=review
> 
> did you measure any improvements from this one?
> 
> > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:-1054
> > -    AXCoreObject::AccessibilityChildrenVector tabs;
> 
> how come we don't need this?
The code builds `tabs` but never uses it, so I believe this is just wasted work unless you know of some reason to do this.
Comment 5 chris fleizach 2023-03-08 16:36:39 PST
Comment on attachment 465365 [details]
Patch

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

>>> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:-1054
>>> -    AXCoreObject::AccessibilityChildrenVector tabs;
>> 
>> how come we don't need this?
> 
> The code builds `tabs` but never uses it, so I believe this is just wasted work unless you know of some reason to do this.

does tabChildren do some work in the background?
Comment 6 Tyler Wilcock 2023-03-08 17:53:18 PST
(In reply to chris fleizach from comment #5)
> Comment on attachment 465365 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=465365&action=review
> 
> >>> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:-1054
> >>> -    AXCoreObject::AccessibilityChildrenVector tabs;
> >> 
> >> how come we don't need this?
> > 
> > The code builds `tabs` but never uses it, so I believe this is just wasted work unless you know of some reason to do this.
> 
> does tabChildren do some work in the background?
The only side-effect it could have is calling AXCoreObject::children(updateChildrenIfNecessary = true), but I don't think that side-effect is needed here (and if it were, a comment should be added, and a test should fail if the side effect is removed).
Comment 7 Tyler Wilcock 2023-03-09 10:26:48 PST
Created attachment 465377 [details]
Patch
Comment 8 EWS 2023-03-10 00:20:34 PST
commit-queue failed to commit attachment 465377 [details] to WebKit repository. To retry, please set cq+ flag again.
Comment 9 EWS 2023-03-10 10:27:30 PST
Committed 261508@main (ae20ce99baa2): <https://commits.webkit.org/261508@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 465377 [details].