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

Darin Adler
Reported 2020-03-17 18:13:59 PDT
Change TextIterator::rangeLength to not require a live range
Attachments
Patch (81.90 KB, patch)
2020-03-17 18:49 PDT, Darin Adler 		no flags
DetailsFormatted DiffDiff
Patch (84.12 KB, patch)
2020-03-18 18:31 PDT, Darin Adler 		no flags
DetailsFormatted DiffDiff
Patch (95.00 KB, patch)
2020-03-21 14:47 PDT, Darin Adler 		no flags
DetailsFormatted DiffDiff
Patch (94.99 KB, patch)
2020-03-21 15:57 PDT, Darin Adler 		no flags
DetailsFormatted DiffDiff
Patch (96.75 KB, patch)
2020-03-22 12:46 PDT, Darin Adler 		no flags
DetailsFormatted DiffDiff
Patch (83.95 KB, patch)
2020-03-22 14:14 PDT, Darin Adler 		no flags
DetailsFormatted DiffDiff
Patch (96.57 KB, patch)
2020-03-22 14:18 PDT, Darin Adler 		koivisto: review+
DetailsFormatted DiffDiff
Patch (97.28 KB, patch)
2020-03-23 12:43 PDT, Darin Adler 		no flags
DetailsFormatted DiffDiff
Show Obsolete (6) View All Add attachment proposed patch, testcase, etc.
Darin Adler
Comment 1 2020-03-17 18:49:28 PDT Comment hidden (obsolete)
Darin Adler
Comment 2 2020-03-18 18:31:03 PDT Comment hidden (obsolete)
Darin Adler
Comment 3 2020-03-21 14:47:25 PDT Comment hidden (obsolete)
Darin Adler
Comment 4 2020-03-21 15:57:04 PDT Comment hidden (obsolete)
Darin Adler
Comment 5 2020-03-22 12:46:33 PDT Comment hidden (obsolete)
Darin Adler
Comment 6 2020-03-22 13:59:57 PDT Comment hidden (obsolete)
Darin Adler
Comment 7 2020-03-22 14:14:27 PDT Comment hidden (obsolete)
Darin Adler
Comment 8 2020-03-22 14:18:14 PDT
Created attachment 394231 [details] Patch
Darin Adler
Comment 9 2020-03-22 17:06:36 PDT
OK, all tests passing. Ready for review now.
Darin Adler
Comment 10 2020-03-22 17:07:08 PDT
Next step on moving off of live ranges.
Antti Koivisto
Comment 11 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.
Darin Adler
Comment 12 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.
Darin Adler
Comment 13 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.
Darin Adler
Comment 14 2020-03-23 12:43:29 PDT
Created attachment 394289 [details] Patch
Darin Adler
Comment 15 2020-03-23 13:50:22 PDT
Committed r258871: <https://trac.webkit.org/changeset/258871>
Radar WebKit Bug Importer
Comment 16 2020-03-23 13:51:15 PDT
<rdar://problem/60790089>
Diego Pino
Comment 17 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
Note You need to log in before you can comment on or make changes to this bug.