Bug 253864

<rdar://problem/106676049>

Created attachment 465427 [details]
Patch

Comment on attachment 465427 [details]
Patch

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

Have you tried running a `sample` with this patch on some text heavy site? I know retrieving text from the render tree / DOM is something that hurt our tree update times badly in the past, so it might be good to make sure we aren't re-introducing that problem as we open this caching up to more types.

> Source/WebCore/accessibility/isolatedtree/mac/AXIsolatedObjectMac.mm:50
> +    // Set the StringValue only if it defers from the AttributedText.

Should "differs" be used instead of "defers" in this comment?

> Source/WebCore/accessibility/isolatedtree/mac/AXIsolatedObjectMac.mm:109
> +    if (auto attributedText = propertyValue<RetainPtr<NSAttributedString>>(AXPropertyName::AttributedText))

Thoughts on using `RetainPtr attributedText` rather than `auto attributedText` so the smart pointer semantics are clear?

> Source/WebCore/accessibility/isolatedtree/mac/AXIsolatedObjectMac.mm:135
> +bool AXIsolatedObject::shouldCacheAttributedText(const Ref<const AccessibilityObject>& axObject) const

Is there any value in this being a reference to a Ref vs. just a & reference?

Also, is it confusing that this is a member function rather than a static function when it doesn't actually use any member properties anymore?

> Source/WebCore/accessibility/isolatedtree/mac/AXIsolatedObjectMac.mm:138
> +        || axObject->isTextControl() || axObject->isTabItem()

Probably won't make any measurable difference, but `isTabItem()` seems least likely to be true, so the branch predictor might benefit if we put it last.

Created attachment 465430 [details]
Patch

(In reply to Tyler Wilcock from comment #3)
> Comment on attachment 465427 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=465427&action=review
> 
> Have you tried running a `sample` with this patch on some text heavy site? I
> know retrieving text from the render tree / DOM is something that hurt our
> tree update times badly in the past, so it might be good to make sure we
> aren't re-introducing that problem as we open this caching up to more types.

I have. Do we even have a choice? One property the client is going to request for sure from a static text object is its text. We either cache it up front, or have to hit the main thread when requested. We were already caching the StringValue text for all objects up front. So this patch now caches the AttributedText instead, with the caveat that in some cases caches both because they are different. This latter limitation needs to be improve to not duplicate the text as much as possible.
> 
> > Source/WebCore/accessibility/isolatedtree/mac/AXIsolatedObjectMac.mm:50
> > +    // Set the StringValue only if it defers from the AttributedText.
> 
> Should "differs" be used instead of "defers" in this comment?

Fixed.
> 
> > Source/WebCore/accessibility/isolatedtree/mac/AXIsolatedObjectMac.mm:109
> > +    if (auto attributedText = propertyValue<RetainPtr<NSAttributedString>>(AXPropertyName::AttributedText))
> 
> Thoughts on using `RetainPtr attributedText` rather than `auto
> attributedText` so the smart pointer semantics are clear?

I think propertyValue<RetainPtr<NSAttributedString>> makes it clear in this case, and replacing auto with RetainPtr does not add anything and degrades legibility.
> 
> > Source/WebCore/accessibility/isolatedtree/mac/AXIsolatedObjectMac.mm:135
> > +bool AXIsolatedObject::shouldCacheAttributedText(const Ref<const AccessibilityObject>& axObject) const
> 
> Is there any value in this being a reference to a Ref vs. just a & reference?
> 
> Also, is it confusing that this is a member function rather than a static
> function when it doesn't actually use any member properties anymore?

Changed it to a static function. Concerning passing Refs as parameters, we started doing that in IsolatedObject methods in 

commit 49c1b53f803fe6952e293bfde206b22dc4ceadef

The convention is that the caller guarantees that the parameter will live for the duration of the called function. So it is fine to pass as parameters raw references and pointers. But since now we have several methods taking Refs, one would need to look through all the callers to make sure that param objects are properly held. I will remove Ref as params in a separate patch.
> 
> > Source/WebCore/accessibility/isolatedtree/mac/AXIsolatedObjectMac.mm:138
> > +        || axObject->isTextControl() || axObject->isTabItem()
> 
> Probably won't make any measurable difference, but `isTabItem()` seems least
> likely to be true, so the branch predictor might benefit if we put it last.

Done, checking for static text and links first since they are the most likely objects.

Committed 261646@main (64cb7c2d3fe3): <https://commits.webkit.org/261646@main>

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