Bug 209207

Summary: Change TextIterator::rangeLength to not require a live range
Product: WebKit Reporter: Darin Adler <darin>
Component: HTML EditingAssignee: Darin Adler <darin>
Status: RESOLVED FIXED    
Severity: Normal CC: aboxhall, andersca, apinheiro, cdumez, cfleizach, dmazzoni, dpino, esprehn+autocc, ews-watchlist, jcraig, jdiggs, kangil.han, koivisto, mifenton, samuel_white, sam, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 209408    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
koivisto: review+
Patch none

Description Darin Adler 2020-03-17 18:13:59 PDT 
Change TextIterator::rangeLength to not require a live range
Comment 1 Darin Adler 2020-03-17 18:49:28 PDT Comment hidden (obsolete)
Comment 2 Darin Adler 2020-03-18 18:31:03 PDT Comment hidden (obsolete)
Comment 3 Darin Adler 2020-03-21 14:47:25 PDT Comment hidden (obsolete)
Comment 4 Darin Adler 2020-03-21 15:57:04 PDT Comment hidden (obsolete)
Comment 5 Darin Adler 2020-03-22 12:46:33 PDT Comment hidden (obsolete)
Comment 6 Darin Adler 2020-03-22 13:59:57 PDT Comment hidden (obsolete)
Comment 7 Darin Adler 2020-03-22 14:14:27 PDT Comment hidden (obsolete)
Comment 8 Darin Adler 2020-03-22 14:18:14 PDT 
Created attachment 394231 [details]
Patch
Comment 9 Darin Adler 2020-03-22 17:06:36 PDT 
OK, all tests passing. Ready for review now.
Comment 10 Darin Adler 2020-03-22 17:07:08 PDT 
Next step on moving off of live ranges.
Comment 11 Antti Koivisto 2020-03-23 05:27:19 PDT 
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.
Comment 12 Darin Adler 2020-03-23 09:03:49 PDT 
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.
Comment 13 Darin Adler 2020-03-23 09:04:33 PDT 
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.
Comment 14 Darin Adler 2020-03-23 12:43:29 PDT 
Created attachment 394289 [details]
Patch
Comment 15 Darin Adler 2020-03-23 13:50:22 PDT 
Committed r258871: <https://trac.webkit.org/changeset/258871>
Comment 16 Radar WebKit Bug Importer 2020-03-23 13:51:15 PDT 
<rdar://problem/60790089>
Comment 17 Diego Pino 2020-03-24 02:12:00 PDT 
GTK test bot is exiting early due to too many crashes after this patch https://bugs.webkit.org/show_bug.cgi?id=209467