Remove live ranges from AccessibilityObject.h, AccessibilityObjectInterface.h, AccessibilityRenderObject.h, AXIsolatedObject.h
Created attachment 404044 [details] Patch
<rdar://problem/65382291>
Created attachment 404046 [details] Patch
Created attachment 404056 [details] Patch
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; });
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.
Created attachment 404068 [details] Patch
Did everything you suggested, and it changed the patch a bit.
Committed r264271: <https://trac.webkit.org/changeset/264271>