Bug 251849 – AX: Remove unnecessary TextMarker methods from WebAccessibilityObjectWrapper

• Component: Accessibility
• Status: RESOLVED FIXED

[Andres Gonzalez]

Use the corresponding C functions instead.

[Radar WebKit Bug Importer]

<rdar://problem/105128584>

[Andres Gonzalez]

Created attachment 464885 [details]
Patch

[Tyler Wilcock]

Comment on attachment 464885 [details]
Patch

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

> COMMIT_MESSAGE:1
> +AX: Remove unnecessary TextMrker methods from WebAccessibilityObjectWrapper. 

Typo — TextMrker should be TextMarker

[Darin Adler]

Comment on attachment 464885 [details]
Patch

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

> COMMIT_MESSAGE:1
> +AX: Remove unnecessary TextMrker methods from WebAccessibilityObjectWrapper. 

Typo here in TextMarker.

> Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:535
> +        [change setObject:marker.bridgingAutorelease() forKey:NSAccessibilityTextChangeValueStartMarker];

Never need autorelease when passing a value to a method, only when returning. Wastefully puts an object into the autorelease pool. Instead this should use get() and bridge_cast. You can probably find an example by searching for bridge_cast.

[Darin Adler]

Set back to review? by accident.

[Andres Gonzalez]

(In reply to Darin Adler from comment #4)
> Comment on attachment 464885 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=464885&action=review
> 
> > COMMIT_MESSAGE:1
> > +AX: Remove unnecessary TextMrker methods from WebAccessibilityObjectWrapper. 
> 
> Typo here in TextMarker.
> 
> > Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:535
> > +        [change setObject:marker.bridgingAutorelease() forKey:NSAccessibilityTextChangeValueStartMarker];
> 
> Never need autorelease when passing a value to a method, only when
> returning. Wastefully puts an object into the autorelease pool. Instead this
> should use get() and bridge_cast. You can probably find an example by
> searching for bridge_cast.

Thanks Darin. Tried that but getting error: no matching function for call to 'bridge_cast'
        [change setObject:bridge_cast(marker.get()) forKey:NSAccessibilityTextChangeValueStartMarker];
                          ^~~~~~~~~~~

/OpenSource/WebKitBuild/Release/usr/local/include/wtf/cocoa/TypeCastsCocoa.h:59:113: note: candidate template ignored: could not match 'RetainPtr<T>' against 'WTF::RetainPtr<const __AXTextMarker>::PtrType' (aka 'const __AXTextMarker *')
template<typename T> inline RetainPtr<std::remove_pointer_t<typename CFTollFreeBridgingTraits<T>::BridgedType>> bridge_cast(RetainPtr<T>&& object)
                                                                                                                ^
/OpenSource/WebKitBuild/Release/usr/local/include/wtf/cocoa/TypeCastsCocoa.h:54:79: note: candidate template ignored: substitution failure [with T = const __AXTextMarker *]: implicit instantiation of undefined template 'WTF::CFTollFreeBridgingTraits<const __AXTextMarker *>'
template<typename T> inline typename CFTollFreeBridgingTraits<T>::BridgedType bridge_cast(T object)
                                     ~~~~~~~~~~~~~~~~~~~~~~~~                 ^
/Users/ag/s/web/OpenSource/WebKitBuild/Release/usr/local/include/wtf/cocoa/TypeCastsCocoa.h:49:113: note: candidate template ignored: could not match 'RetainPtr<T>' against 'WTF::RetainPtr<const __AXTextMarker>::PtrType' (aka 'const __AXTextMarker *')
template<typename T> inline RetainPtr<typename NSTollFreeBridgingTraits<std::remove_pointer_t<T>>::BridgedType> bridge_cast(RetainPtr<T>&& object)
                                                                                                                ^
/Users/ag/s/web/OpenSource/WebKitBuild/Release/usr/local/include/wtf/cocoa/TypeCastsCocoa.h:44:102: note: candidate template ignored: substitution failure [with T = const __AXTextMarker *]: implicit instantiation of undefined template 'WTF::NSTollFreeBridgingTraits<const __AXTextMarker>'
template<typename T> inline typename NSTollFreeBridgingTraits<std::remove_pointer_t<T>>::BridgedType bridge_cast(T object)
                                     ~~~~~~~~~~~~~~~~~~~~~~~~                                        ^

Maybe because AXTextMarkerRef is an opaque pointer?


Can get it to compile with:

        [change setObject:(__bridge id)(marker.get()) forKey:NSAccessibilityTextChangeValueStartMarker];

which perhaps is the right thing in this case.

[Andres Gonzalez]

Created attachment 464923 [details]
Patch

[Andres Gonzalez]

Created attachment 464924 [details]
Patch

[Darin Adler]

Comment on attachment 464924 [details]
Patch

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

> Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:535
> +        [change setObject:(__bridge id)marker.get() forKey:NSAccessibilityTextChangeValueStartMarker];

Great as is. To make code like this look more elegant and perhaps be more type safe we also have bridge_id_cast for cases where there is not an Objective-C equivalent. Or if there is an Objective-C toll free bridged version of AXTextMarkerRef, we could add it to the toll free bridging header and then use bridge_cast. But none of that is required, this is fine.

[EWS]

Committed 260075@main (cf1fe7b77825): <https://commits.webkit.org/260075@main>

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