Bug 214053 – Remove use of live ranges from AXObject.h

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Darin Adler]

Remove use of live ranges from AXObject.h

[Darin Adler]

Created attachment 403715 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/65189732>

[Darin Adler]

Created attachment 403737 [details]
Patch

[Sam Weinig]

Comment on attachment 403737 [details]
Patch

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

> Source/WebCore/accessibility/AXObjectCache.cpp:2046
> +static bool setRangeStartOrEndWithCharacterOffset(SimpleRange& range, const CharacterOffset& characterOffset, bool isStart)

Not new, but the bool isStart here makes the call below harder to follow than needed.  Could do with an enum replacement. (This bool isStart is common pattern in this file).

> Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:2813
> +    int length = AXObjectCache::lengthForRange(SimpleRange { *range });
>      return length < 0 ? 0 : length;

std::max(0, length);?

[Darin Adler]

Comment on attachment 403737 [details]
Patch

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

>> Source/WebCore/accessibility/AXObjectCache.cpp:2046
>> +static bool setRangeStartOrEndWithCharacterOffset(SimpleRange& range, const CharacterOffset& characterOffset, bool isStart)
> 
> Not new, but the bool isStart here makes the call below harder to follow than needed.  Could do with an enum replacement. (This bool isStart is common pattern in this file).

I agree. My original patch eliminated this function entirely and cut down a ton on use of booleans for isStart, but it was too much change for my goal: move from live range to SimpleRange, so I decided to preserve this function as is instead. There’s a lot more simplification that can be done here but I am holding back on it for now.

>> Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:2813
>>      return length < 0 ? 0 : length;
> 
> std::max(0, length);?

Sure, that would be better, but I think I will leave this alone for now. There’s actually no reason for AXObjectCache::lengthForRange to ever return a negative number as far as I can tell. It returns -1 instead of 0 in some error conditions, but I can’t find any code taking advantage of that. Could simply change its return type and get rid of code like this.

[EWS]

Committed r264118: <https://trac.webkit.org/changeset/264118>

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