Bug 209207 – Change TextIterator::rangeLength to not require a live range

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Darin Adler]

Change TextIterator::rangeLength to not require a live range

[Darin Adler]

Created attachment 393808 [details]
Patch

[Darin Adler]

Created attachment 393931 [details]
Patch

[Darin Adler]

Created attachment 394179 [details]
Patch

[Darin Adler]

Created attachment 394182 [details]
Patch

[Darin Adler]

Created attachment 394226 [details]
Patch

[Darin Adler]

Comment on attachment 394226 [details]
Patch

WPE build still failing

[Darin Adler]

Created attachment 394230 [details]
Patch

[Darin Adler]

Created attachment 394231 [details]
Patch

[Darin Adler]

OK, all tests passing. Ready for review now.

[Darin Adler]

Next step on moving off of live ranges.

[Antti Koivisto]

Comment on attachment 394231 [details]
Patch

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

> Source/WebCore/accessibility/AXObjectCache.cpp:1946
> -    auto searchRange = Range::create(m_document, startPosition, endPosition);
> -    if (searchRange->collapsed())
> +    SimpleRange searchRange { *makeBoundaryPoint(startPosition), *makeBoundaryPoint(endPosition) };

I sort of like 

auto searchRange = SimpleRange { *makeBoundaryPoint(startPosition), *makeBoundaryPoint(endPosition) };

for these. It is longer but I feel it reads better. It is also allows replacing constructor call with a function call (or opposite, like here) with minimal changes.

> Source/WebCore/editing/TextIterator.h:299
> +WEBCORE_EXPORT CharacterCount characterCount(const SimpleRange&, bool spacesForReplacedElements = false);

The boolean is rather magical in the call sites. Please add enum.

[Darin Adler]

Comment on attachment 394231 [details]
Patch

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

>> Source/WebCore/accessibility/AXObjectCache.cpp:1946
>> +    SimpleRange searchRange { *makeBoundaryPoint(startPosition), *makeBoundaryPoint(endPosition) };
> 
> I sort of like 
> 
> auto searchRange = SimpleRange { *makeBoundaryPoint(startPosition), *makeBoundaryPoint(endPosition) };
> 
> for these. It is longer but I feel it reads better. It is also allows replacing constructor call with a function call (or opposite, like here) with minimal changes.

OK. That does seem better.

[Darin Adler]

Comment on attachment 394231 [details]
Patch

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

>> Source/WebCore/editing/TextIterator.h:299
>> +WEBCORE_EXPORT CharacterCount characterCount(const SimpleRange&, bool spacesForReplacedElements = false);
> 
> The boolean is rather magical in the call sites. Please add enum.

Will do. Might want to do an OptionSet later, but for now an enum class should do.

[Darin Adler]

Created attachment 394289 [details]
Patch

[Darin Adler]

Committed r258871: <https://trac.webkit.org/changeset/258871>

[Radar WebKit Bug Importer]

<rdar://problem/60790089>

[Diego Pino]

GTK test bot is exiting early due to too many crashes after this patch https://bugs.webkit.org/show_bug.cgi?id=209467