Bug 236115

Summary: Cache some expensive AXIsolatedObject properties lazily.
Product: WebKit Reporter: Andres Gonzalez <andresg_22>
Component: AccessibilityAssignee: Andres Gonzalez <andresg_22>
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: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

Description Andres Gonzalez 2022-02-03 18:26:41 PST 
Cache some expensive AXIsolatedObject properties lazily.
Comment 1 Radar WebKit Bug Importer 2022-02-03 18:26:54 PST 
<rdar://problem/88467667>
Comment 2 Andres Gonzalez 2022-02-03 18:45:59 PST 
Created attachment 450850 [details]
Patch
Comment 3 chris fleizach 2022-02-03 19:59:06 PST 
Comment on attachment 450850 [details]
Patch

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

> Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp:978
> +String AXIsolatedObject::getOrRetrieveStringPropertyValue(AXPropertyName propertyName)

Get and retrieve seem like synonyms here so you could probably drop both and just call it stringPropertyValue()

> Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp:1009
> +        setProperty(propertyName, value.isolatedCopy());

should we cache if the string is empty
Comment 4 Andres Gonzalez 2022-02-04 05:16:54 PST 
(In reply to chris fleizach from comment #3)
> Comment on attachment 450850 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=450850&action=review
> 
> > Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp:978
> > +String AXIsolatedObject::getOrRetrieveStringPropertyValue(AXPropertyName propertyName)
> 
> Get and retrieve seem like synonyms here so you could probably drop both and
> just call it stringPropertyValue()

The idea is to differentiate it from stringAttributeValue that I intend to rename stringPropertyValue. The getOrRetrieve makes explicit that the value is either gotten from the cache or retrieved from the main thread. The reason I'd like to rename stringAttributeValue -> stringPropertyValue is the confusing overload of "attribute" which is used for HTML element attributes, platform API methods, and AXCoreObject methods.

> 
> > Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp:1009
> > +        setProperty(propertyName, value.isolatedCopy());
> 
> should we cache if the string is empty

I think so since that the behavior of the live object as well.
Comment 5 chris fleizach 2022-02-04 07:25:40 PST 
Comment on attachment 450850 [details]
Patch

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

>>> Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp:978
>>> +String AXIsolatedObject::getOrRetrieveStringPropertyValue(AXPropertyName propertyName)
>> 
>> Get and retrieve seem like synonyms here so you could probably drop both and just call it stringPropertyValue()
> 
> The idea is to differentiate it from stringAttributeValue that I intend to rename stringPropertyValue. The getOrRetrieve makes explicit that the value is either gotten from the cache or retrieved from the main thread. The reason I'd like to rename stringAttributeValue -> stringPropertyValue is the confusing overload of "attribute" which is used for HTML element attributes, platform API methods, and AXCoreObject methods.

In this case maybe cachedStringPropertyValue would be good
Comment 6 EWS 2022-02-04 11:20:59 PST 
Committed r289131 (246828@main): <https://commits.webkit.org/246828@main>

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