Bug 208906 - Change all return values in TextIterator header from live ranges to SimpleRange
Summary: Change all return values in TextIterator header from live ranges to SimpleRange
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Darin Adler
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-03-10 22:49 PDT by Darin Adler
Modified: 2020-03-15 12:36 PDT (History)
18 users (show)

See Also:


Attachments
Patch (44.92 KB, patch)
2020-03-10 23:06 PDT, Darin Adler
koivisto: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2020-03-10 22:49:46 PDT
Change all return values in TextIterator header from live ranges to SimpleRange
Comment 1 Darin Adler 2020-03-10 23:06:09 PDT
Created attachment 393207 [details]
Patch
Comment 2 Darin Adler 2020-03-11 08:22:50 PDT
Hooray this one passed all the tests on the first try
Comment 3 Antti Koivisto 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
Comment 4 Darin Adler 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.
Comment 5 Darin Adler 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.
Comment 6 Darin Adler 2020-03-14 15:59:39 PDT
Committed r258475: <https://trac.webkit.org/changeset/258475>
Comment 7 Radar WebKit Bug Importer 2020-03-14 16:00:16 PDT
<rdar://problem/60460158>
Comment 8 Antti Koivisto 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".
Comment 9 Antti Koivisto 2020-03-15 01:07:52 PDT
...and doesn't alter the behavior.
Comment 10 Darin Adler 2020-03-15 12:36:41 PDT
Consider it done, then!