Bug 254817

<rdar://problem/107473776>

Created attachment 465701 [details]
Patch

Comment on attachment 465701 [details]
Patch

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

> Source/WebCore/accessibility/AXTextMarker.cpp:158
> +    bool isInNode = static_cast<unsigned>(characterCount) <= WebCore::characterCount(nodeRange);

Are you confident this cast from int to unsigned is always safe? Should we guard against a negative `characterCount`, or is it possible to make the characterCount parameter an unsigned and move the cast somewhere with more context?

> Source/WebCore/accessibility/AXTextMarker.cpp:172
> +    Node* node = characterOffset.node;

AccessibilityObject::replacedNodeNeedsCharacter can call accessibilityIsIgnored, which can do work that destroys `node`. Do we want to make this a WeakPtr<Node> instead of a raw pointer?

It would be nice if it could be a CheckedPtr (because it's cheaper than all other smart pointers), but it doesn't look like you can make CheckedPtr<Node>.

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2841
> +        AXTextMarkerRange markerRange { marker, marker };

Just making sure this is correct and not a typo -- the intention here is to make a range starting from and ending at `marker`?

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2843
> +        return range ? makeNSRange(range).location : NSNotFound;

Is it necessary to null-check range? makeNSRange takes a std::optional already.

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3259
> +        return (id)Accessibility::retrieveAutoreleasedValueFromMainThread<AXTextMarkerRangeRef>([&dictionary, protectedSelf = retainPtr(self)] () -> RetainPtr<AXTextMarkerRangeRef> {

This lambda claims:

-> RetainPtr<AXTextMarkerRangeRef>

But doesn't have RetainPtr in the first type-specifier:

accessibility::retrieveAutoreleasedValueFromMainThread<AXTextMarkerRangeRef>

Is that OK?

> LayoutTests/accessibility/text-marker/text-marker-previous-next.html:232
> +        markerRange = text.textMarkerRangeForMarkers(currentMarker,previousMarker);

Missing a space after the comma.

(In reply to Tyler Wilcock from comment #3)
> Comment on attachment 465701 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=465701&action=review
> 
> > Source/WebCore/accessibility/AXTextMarker.cpp:158
> > +    bool isInNode = static_cast<unsigned>(characterCount) <= WebCore::characterCount(nodeRange);
> 
> Are you confident this cast from int to unsigned is always safe? Should we
> guard against a negative `characterCount`, or is it possible to make the
> characterCount parameter an unsigned and move the cast somewhere with more
> context?

Not safe, this is old, eerie CharacterOffset code copied from AXObjectCache.cpp, see resetNodeAndOffsetForReplacedNode. The goal is to get rid off all the CharacterOffset stuff, but it has to be one step at a time to keep things working. I'll address these issues with the CharacterOffset implementation in subsequent patches.
> 
> > Source/WebCore/accessibility/AXTextMarker.cpp:172
> > +    Node* node = characterOffset.node;
> 
> AccessibilityObject::replacedNodeNeedsCharacter can call
> accessibilityIsIgnored, which can do work that destroys `node`. Do we want
> to make this a WeakPtr<Node> instead of a raw pointer?
> 
> It would be nice if it could be a CheckedPtr (because it's cheaper than all
> other smart pointers), but it doesn't look like you can make
> CheckedPtr<Node>.

Fixed.
> 
> > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2841
> > +        AXTextMarkerRange markerRange { marker, marker };
> 
> Just making sure this is correct and not a typo -- the intention here is to
> make a range starting from and ending at `marker`?

Yes, it is a collapsed range, start = end.
> 
> > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2843
> > +        return range ? makeNSRange(range).location : NSNotFound;
> 
> Is it necessary to null-check range? makeNSRange takes a std::optional
> already.

Fixed, made it more concise.
> 
> > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3259
> > +        return (id)Accessibility::retrieveAutoreleasedValueFromMainThread<AXTextMarkerRangeRef>([&dictionary, protectedSelf = retainPtr(self)] () -> RetainPtr<AXTextMarkerRangeRef> {
> 
> This lambda claims:
> 
> -> RetainPtr<AXTextMarkerRangeRef>
> 
> But doesn't have RetainPtr in the first type-specifier:
> 
> accessibility::retrieveAutoreleasedValueFromMainThread<AXTextMarkerRangeRef>
> 
> Is that OK?

Yes, it is correct, the returned value of retrieveAutoreleased... is T* but the return value of the lambda argument has to be RetainPtr<T> so that it can be autoreleased.
> 
> > LayoutTests/accessibility/text-marker/text-marker-previous-next.html:232
> > +        markerRange = text.textMarkerRangeForMarkers(currentMarker,previousMarker);
> 
> Missing a space after the comma.

Fixed. Thanks!

Created attachment 465712 [details]
Patch

Committed 262480@main (5ffe861ef145): <https://commits.webkit.org/262480@main>

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