Bug 214053 - Remove use of live ranges from AXObject.h
Summary: Remove use of live ranges from AXObject.h
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Darin Adler
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-07-07 12:57 PDT by Darin Adler
Modified: 2020-07-08 11:03 PDT (History)
13 users (show)

See Also:


Attachments
Patch (48.07 KB, patch)
2020-07-07 13:04 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (52.51 KB, patch)
2020-07-07 15:58 PDT, Darin Adler
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2020-07-07 12:57:14 PDT
Remove use of live ranges from AXObject.h
Comment 1 Darin Adler 2020-07-07 13:04:47 PDT Comment hidden (obsolete)
Comment 2 Radar WebKit Bug Importer 2020-07-07 13:05:19 PDT
<rdar://problem/65189732>
Comment 3 Darin Adler 2020-07-07 15:58:09 PDT
Created attachment 403737 [details]
Patch
Comment 4 Sam Weinig 2020-07-07 18:14:42 PDT
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);?
Comment 5 Darin Adler 2020-07-08 10:55:42 PDT
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.
Comment 6 EWS 2020-07-08 11:03:26 PDT
Committed r264118: <https://trac.webkit.org/changeset/264118>

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