AX: The platform wrapper code is unnecessarily creating an additional SimpleRange when calculating a TextMarkerRange length.

• Component: Accessibility
• Status: RESOLVED FIXED

[Andres Gonzalez]

.

[Radar WebKit Bug Importer]

<rdar://problem/107335186>

[Andres Gonzalez]

Created attachment 465643 [details]
Patch

[Tyler Wilcock]

Comment on attachment 465643 [details]
Patch

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

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3583
> +        unsigned length = Accessibility::retrieveValueFromMainThread<int>([textMarkerRange = retainPtr(textMarkerRange), protectedSelf = retainPtr(self)] () -> int {

This declares that it returns `-> int`, but it now should be `-> unsigned`, right?

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3591
> -        if (length < 0)
> -            return nil;
>          return @(length);

Prior to your patch, we would return -1 for std::nullopt ranges. Now that `lengthForRange` returns unsigned, we will never return `nil` for this attribute. Is this behavior change OK?

[Andres Gonzalez]

Created attachment 465648 [details]
Patch

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #3)
> Comment on attachment 465643 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=465643&action=review
> 
> > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3583
> > +        unsigned length = Accessibility::retrieveValueFromMainThread<int>([textMarkerRange = retainPtr(textMarkerRange), protectedSelf = retainPtr(self)] () -> int {
> 
> This declares that it returns `-> int`, but it now should be `-> unsigned`,
> right?

Yes, fixed.
> 
> > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3591
> > -        if (length < 0)
> > -            return nil;
> >          return @(length);
> 
> Prior to your patch, we would return -1 for std::nullopt ranges. Now that
> `lengthForRange` returns unsigned, we will never return `nil` for this
> attribute. Is this behavior change OK?

I believe so, no client should expect a -1 length.

[Andres Gonzalez]

(In reply to Andres Gonzalez from comment #5)
> (In reply to Tyler Wilcock from comment #3)
> > Comment on attachment 465643 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=465643&action=review
> > 
> > > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3583
> > > +        unsigned length = Accessibility::retrieveValueFromMainThread<int>([textMarkerRange = retainPtr(textMarkerRange), protectedSelf = retainPtr(self)] () -> int {
> > 
> > This declares that it returns `-> int`, but it now should be `-> unsigned`,
> > right?
> 
> Yes, fixed.
> > 
> > > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3591
> > > -        if (length < 0)
> > > -            return nil;
> > >          return @(length);
> > 
> > Prior to your patch, we would return -1 for std::nullopt ranges. Now that
> > `lengthForRange` returns unsigned, we will never return `nil` for this
> > attribute. Is this behavior change OK?
> 
> I believe so, no client should expect a -1 length.

or rather a nil when requesting the length of a range.

[EWS]

Committed 262270@main (3a1d73db387c): <https://commits.webkit.org/262270@main>

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