Bug 214215 – Remove live ranges from AccessibilityObject.h, AccessibilityObjectInterface.h, AccessibilityRenderObject.h, AXIsolatedObject.h

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

[Darin Adler]

Remove live ranges from AccessibilityObject.h, AccessibilityObjectInterface.h, AccessibilityRenderObject.h, AXIsolatedObject.h

[Darin Adler]

Created attachment 404044 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/65382291>

[Darin Adler]

Created attachment 404046 [details]
Patch

[Darin Adler]

Created attachment 404056 [details]
Patch

[Sam Weinig]

Comment on attachment 404056 [details]
Patch

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

> Source/WebCore/accessibility/AccessibilityObject.cpp:309
> +            if (misspellingRange->compareBoundaryPoints(Range::END_TO_END, createLiveRange(start)).releaseReturnValue() > 0)

It's a bit unfortunate that this live range needs to get created each time through the loop. Might be worth caching it until we have a version of compareBoundaryPoints that can take a SimpleRange.

> Source/WebCore/accessibility/AccessibilityObject.cpp:1278
> +    auto start = makeBoundaryPoint(visiblePositionRange.start);
> +    auto end = makeBoundaryPoint(visiblePositionRange.end);
> +    if (!start || !end)
>          return String();

I'd probably break this up into:

    auto start = makeBoundaryPoint(visiblePositionRange.start);
    if (!start)
        return { };
    auto end = makeBoundaryPoint(visiblePositionRange.end);
    if (!end)
        return { };

to skip the unnecessary construction of end if start is null.

> Source/WebCore/accessibility/AccessibilityObject.cpp:1281
> +    for (TextIterator it({ *start, *end }); !it.atEnd(); it.advance()) {

One day, I will quietly turn this into a proper c++ style iterator.

> Source/WebCore/accessibility/AccessibilityObject.cpp:1303
> +    auto start = makeBoundaryPoint(visiblePositionRange.start);
> +    auto end = makeBoundaryPoint(visiblePositionRange.end);
> +    if (!start || !end)
> +        return -1; // FIXME: Why not return 0?

Same comment about splitting this up (and in a few more places below.

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:917
> +        auto start = VisiblePosition { createLegacyEditingPosition(range->start) };

This could be moved within the if-statement below.

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2170
> +    CharacterOffset start = cache->startOrEndCharacterOffsetForRange(range, true);
> +    CharacterOffset end = cache->startOrEndCharacterOffsetForRange(range, false);

Could use auto here. function name makes it very clear what the return type is.

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:698
> +        operation.textRanges.reserveInitialCapacity(markerRanges.count);
> +        for (id markerRange : markerRanges) {
> +            if (auto range = [obj rangeForTextMarkerRange:markerRange])
> +                operation.textRanges.append(*range);
> +        }

This could be slightly more optimal by using uncheckedAppend. I don't think we have a map() for this case, but it would be nice if you could write something like:

operation.textRanges = WTF::mapIgnoringNullopts(markerRanges, [] (const auto& v) -> Optional<SimpleRange> { 
     if (auto range = [obj rangeForTextMarkerRange:markerRange])
         return *range;
    return WTF::nullopt;   
});

[Darin Adler]

Comment on attachment 404056 [details]
Patch

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

I guess I still have a crash to fix.

>> Source/WebCore/accessibility/AccessibilityObject.cpp:309
>> +            if (misspellingRange->compareBoundaryPoints(Range::END_TO_END, createLiveRange(start)).releaseReturnValue() > 0)
> 
> It's a bit unfortunate that this live range needs to get created each time through the loop. Might be worth caching it until we have a version of compareBoundaryPoints that can take a SimpleRange.

I’ve been pretty cavalier about such things, often valuing clarity of future refactoring over efficiency. I’m fine hosting it out of the loop in this case, though.

>> Source/WebCore/accessibility/AccessibilityObject.cpp:1278
>>          return String();
> 
> I'd probably break this up into:
> 
>     auto start = makeBoundaryPoint(visiblePositionRange.start);
>     if (!start)
>         return { };
>     auto end = makeBoundaryPoint(visiblePositionRange.end);
>     if (!end)
>         return { };
> 
> to skip the unnecessary construction of end if start is null.

I’d be willing to do this, but I intentionally used the more economical style, even though it’s less efficient for the null case.

>> Source/WebCore/accessibility/AccessibilityObject.cpp:1281
>> +    for (TextIterator it({ *start, *end }); !it.atEnd(); it.advance()) {
> 
> One day, I will quietly turn this into a proper c++ style iterator.

Yes, making some style of TextIterator work in a range-based for loop would not even be all that hard!

>> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:917
>> +        auto start = VisiblePosition { createLegacyEditingPosition(range->start) };
> 
> This could be moved within the if-statement below.

It can’t, because end is computed based on start.

>> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2170
>> +    CharacterOffset end = cache->startOrEndCharacterOffsetForRange(range, false);
> 
> Could use auto here. function name makes it very clear what the return type is.

OK.

>> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:698
>> +        }
> 
> This could be slightly more optimal by using uncheckedAppend. I don't think we have a map() for this case, but it would be nice if you could write something like:
> 
> operation.textRanges = WTF::mapIgnoringNullopts(markerRanges, [] (const auto& v) -> Optional<SimpleRange> { 
>      if (auto range = [obj rangeForTextMarkerRange:markerRange])
>          return *range;
>     return WTF::nullopt;   
> });

I think we are *very* close to having a map() for this case. We have makeVector in VectorCocoa.h that does things like this. However, unlike the createNSArray function in the same file, it does yet not have a version that takes a mapping function, so I would have to overload makeVectorElement to use it.

[Darin Adler]

Created attachment 404068 [details]
Patch

[Darin Adler]

Did everything you suggested, and it changed the patch a bit.

[Darin Adler]

Committed r264271: <https://trac.webkit.org/changeset/264271>