Bug 209207 - Change TextIterator::rangeLength to not require a live range
Summary: Change TextIterator::rangeLength to not require a live range
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Darin Adler
URL:
Keywords: InRadar
Depends on:
Blocks: 209408
  Show dependency tree / graph
 
Reported: 2020-03-17 18:13 PDT by Darin Adler
Modified: 2020-03-24 02:12 PDT (History)
18 users (show)

See Also:

Attachments
Patch (81.90 KB, patch)
2020-03-17 18:49 PDT, Darin Adler 		no flags Details | Formatted Diff | Diff
Patch (84.12 KB, patch)
2020-03-18 18:31 PDT, Darin Adler 		no flags Details | Formatted Diff | Diff
Patch (95.00 KB, patch)
2020-03-21 14:47 PDT, Darin Adler 		no flags Details | Formatted Diff | Diff
Patch (94.99 KB, patch)
2020-03-21 15:57 PDT, Darin Adler 		no flags Details | Formatted Diff | Diff
Patch (96.75 KB, patch)
2020-03-22 12:46 PDT, Darin Adler 		no flags Details | Formatted Diff | Diff
Patch (83.95 KB, patch)
2020-03-22 14:14 PDT, Darin Adler 		no flags Details | Formatted Diff | Diff
Patch (96.57 KB, patch)
2020-03-22 14:18 PDT, Darin Adler 		koivisto: review+
Details | Formatted Diff | Diff
Patch (97.28 KB, patch)
2020-03-23 12:43 PDT, Darin Adler 		no flags Details | Formatted Diff | Diff
Show Obsolete (6) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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