RESOLVED FIXED 214215
Remove live ranges from AccessibilityObject.h, AccessibilityObjectInterface.h, AccessibilityRenderObject.h, AXIsolatedObject.h
https://bugs.webkit.org/show_bug.cgi?id=214215
Summary Remove live ranges from AccessibilityObject.h, AccessibilityObjectInterface.h...
Darin Adler
Reported 2020-07-10 21:42:59 PDT
Remove live ranges from AccessibilityObject.h, AccessibilityObjectInterface.h, AccessibilityRenderObject.h, AXIsolatedObject.h
Attachments
Patch (60.50 KB, patch)
2020-07-10 21:55 PDT, Darin Adler
no flags
Patch (63.75 KB, patch)
2020-07-10 22:25 PDT, Darin Adler
no flags
Patch (63.91 KB, patch)
2020-07-11 09:47 PDT, Darin Adler
sam: review+
Patch (76.95 KB, patch)
2020-07-11 13:48 PDT, Darin Adler
no flags
Darin Adler
Comment 1 2020-07-10 21:55:46 PDT Comment hidden (obsolete)
Radar WebKit Bug Importer
Comment 2 2020-07-10 21:56:16 PDT
Darin Adler
Comment 3 2020-07-10 22:25:42 PDT Comment hidden (obsolete)
Darin Adler
Comment 4 2020-07-11 09:47:33 PDT
Sam Weinig
Comment 5 2020-07-11 10:27:23 PDT
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 6 2020-07-11 11:50:47 PDT
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
Comment 7 2020-07-11 13:48:38 PDT
Darin Adler
Comment 8 2020-07-11 13:49:12 PDT
Did everything you suggested, and it changed the patch a bit.
Darin Adler
Comment 9 2020-07-11 15:38:51 PDT
Note You need to log in before you can comment on or make changes to this bug.