[webkit-reviews] review granted: [Bug 214215] Remove live ranges from AccessibilityObject.h, AccessibilityObjectInterface.h, AccessibilityRenderObject.h, AXIsolatedObject.h : [Attachment 404056] Patch

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has granted Darin Adler <darin at apple.com>'s request
for review:
Bug 214215: Remove live ranges from AccessibilityObject.h,
AccessibilityObjectInterface.h, AccessibilityRenderObject.h, AXIsolatedObject.h
https://bugs.webkit.org/show_bug.cgi?id=214215

Attachment 404056: Patch

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




--- Comment #5 from Sam Weinig <sam at webkit.org> ---
Comment on attachment 404056
  --> https://bugs.webkit.org/attachment.cgi?id=404056
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;   
});