Bug 208906

Summary:

Change all return values in TextIterator header from live ranges to SimpleRange

Product:

WebKit

Reporter:

Darin Adler <darin>

Component:

HTML Editing

Assignee:

Darin Adler <darin>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

aboxhall, andersca, apinheiro, cdumez, cfleizach, dmazzoni, don.olmstead, esprehn+autocc, ews-watchlist, jcraig, jdiggs, kangil.han, koivisto, mifenton, samuel_white, sam, webkit-bug-importer, wenson_hsieh

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

All

OS:

All

Attachments:

Description	Flags
Patch	koivisto: review+

Darin Adler

Reported 2020-03-10 22:49:46 PDT

Change all return values in TextIterator header from live ranges to SimpleRange

Attachments
Patch (44.92 KB, patch) 2020-03-10 23:06 PDT, Darin Adler	koivisto: review+	Details Formatted Diff Diff
View All Add attachment proposed patch, testcase, etc.

Darin Adler

Comment 1 2020-03-10 23:06:09 PDT

Created attachment 393207 [details] Patch

Darin Adler

Comment 2 2020-03-11 08:22:50 PDT

Hooray this one passed all the tests on the first try

Antti Koivisto

Comment 3 2020-03-14 03:05:42 PDT

Comment on attachment 393207 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=393207&action=review > Source/WebCore/accessibility/AXObjectCache.cpp:2667 > + Node& node = it.atEnd() ? searchRange->startContainer() : it.range().start.container.get(); auto > Source/WebCore/editing/TextIterator.cpp:1494 > - Ref<Range> r = m_underlyingIterator.range(); > + SimpleRange range = m_underlyingIterator.range(); auto > Source/WebCore/editing/TextIterator.cpp:1499 > + Node& node = range.start.container; auto > Source/WebCore/editing/TextIterator.cpp:2508 > + for (TextIterator iterator { createLiveRange(range).ptr(), behavior }; !iterator.atEnd(); iterator.advance()) { I suppose the plan to also make TextIterator take SimpleRange at some point? > Source/WebCore/editing/VisibleUnits.cpp:638 > + Node& node = it.atEnd() ? searchRange->startContainer() : it.range().start.container.get(); auto > Source/WebCore/editing/VisibleUnits.cpp:649 > + return VisiblePosition(createLegacyEditingPosition(charIt.range().end), DOWNSTREAM); Could just use { } > Source/WebCore/editing/VisibleUnits.cpp:685 > + SimpleRange characterRange = charIt.range(); auto

Darin Adler

Comment 4 2020-03-14 15:54:39 PDT

Comment on attachment 393207 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=393207&action=review Thanks very much for the review. >> Source/WebCore/accessibility/AXObjectCache.cpp:2667 >> + Node& node = it.atEnd() ? searchRange->startContainer() : it.range().start.container.get(); > > auto You mean auto, and not auto&? For now I did auto&. >> Source/WebCore/editing/TextIterator.cpp:1494 >> + SimpleRange range = m_underlyingIterator.range(); > > auto Done. >> Source/WebCore/editing/TextIterator.cpp:1499 >> + Node& node = range.start.container; > > auto Did auto& for now. >> Source/WebCore/editing/TextIterator.cpp:2508 >> + for (TextIterator iterator { createLiveRange(range).ptr(), behavior }; !iterator.atEnd(); iterator.advance()) { > > I suppose the plan to also make TextIterator take SimpleRange at some point? Yes, that's right. Just doing the changes one step at a time, but will get to that soon. >> Source/WebCore/editing/VisibleUnits.cpp:638 >> + Node& node = it.atEnd() ? searchRange->startContainer() : it.range().start.container.get(); > > auto Did auto&. >> Source/WebCore/editing/VisibleUnits.cpp:649 >> + return VisiblePosition(createLegacyEditingPosition(charIt.range().end), DOWNSTREAM); > > Could just use { } Trying that (waiting for it to compile right now). >> Source/WebCore/editing/VisibleUnits.cpp:685 >> + SimpleRange characterRange = charIt.range(); > > auto Done.

Darin Adler

Comment 5 2020-03-14 15:55:39 PDT

Comment on attachment 393207 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=393207&action=review >>> Source/WebCore/editing/TextIterator.cpp:1499 >>> + Node& node = range.start.container; >> >> auto > > Did auto& for now. Turns out this one needs to be Node&; changed it back.

Darin Adler

Comment 6 2020-03-14 15:59:39 PDT

Committed r258475: <https://trac.webkit.org/changeset/258475>

Radar WebKit Bug Importer

Comment 7 2020-03-14 16:00:16 PDT

<rdar://problem/60460158>

Antti Koivisto

Comment 8 2020-03-15 01:06:28 PDT

> You mean auto, and not auto&? For now I did auto&. Right, I mean "auto in a way that builds and follows WebKit coding style".

Antti Koivisto

Comment 9 2020-03-15 01:07:52 PDT

...and doesn't alter the behavior.

Darin Adler

Comment 10 2020-03-15 12:36:41 PDT

Consider it done, then!

Note You need to log in before you can comment on or make changes to this bug.