Bug 254620

Summary: AX: The platform wrapper code is unnecessarily creating an additional SimpleRange when calculating a TextMarkerRange length.
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, tyler_w, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Patch none

Description Andres Gonzalez 2023-03-28 14:26:17 PDT
.
Comment 1 Radar WebKit Bug Importer 2023-03-28 14:26:45 PDT
<rdar://problem/107335186>
Comment 2 Andres Gonzalez 2023-03-28 14:40:13 PDT
Created attachment 465643 [details]
Patch
Comment 3 Tyler Wilcock 2023-03-28 15:55:49 PDT
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?
Comment 4 Andres Gonzalez 2023-03-28 17:38:05 PDT
Created attachment 465648 [details]
Patch
Comment 5 Andres Gonzalez 2023-03-28 17:40:33 PDT
(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.
Comment 6 Andres Gonzalez 2023-03-28 17:42:04 PDT
(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.
Comment 7 EWS 2023-03-29 06:05:46 PDT
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].