Bug 209723 – Remove all uses of live ranges from TextIterator

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Darin Adler]

Remove all uses of live ranges from TextIterator

[Darin Adler]

Created attachment 394885 [details]
Patch

[Darin Adler]

Created attachment 394886 [details]
Patch

[Darin Adler]

Created attachment 395221 [details]
Patch

[Darin Adler]

Created attachment 395229 [details]
Patch

[Darin Adler]

Created attachment 395241 [details]
Patch

[Darin Adler]

After this patch, TextIterator is done with live ranges and we can move on to removing other uses of them. Might want to fix VisibleSelection next, or simply search for calls to Range::create and createLiveRange and fix them one at a time.

[Darin Adler]

Antti, would you be willing to review this one? All the tests are passing.

[Antti Koivisto]

Comment on attachment 395241 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=395241&action=review

Looks good.

> Source/WebCore/accessibility/AccessibilityObject.cpp:1462
> +    String lineString = plainText(*makeRange(startOfLine(nextVisiblePos), endOfLine(nextVisiblePos)));

The old code null checked the range. This just dereferenced without checks. Why is that correct? 

This could use auto.

> Source/WebCore/accessibility/AccessibilityObject.cpp:1486
> +    String lineString = plainText(*makeRange(startOfLine(previousVisiblePos), endOfLine(previousVisiblePos)));

Similarly.

[Darin Adler]

Comment on attachment 395241 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=395241&action=review

Thanks for the review.

>> Source/WebCore/accessibility/AccessibilityObject.cpp:1462
>> +    String lineString = plainText(*makeRange(startOfLine(nextVisiblePos), endOfLine(nextVisiblePos)));
> 
> The old code null checked the range. This just dereferenced without checks. Why is that correct? 
> 
> This could use auto.

I thought the answer was that makeRange can only return null if it’s passed null and this function never passes null. But there is maybe a tiny window of uncertainty because maybe somehow a VisiblePosition that is not null could still cause startOfLine to return null (seems unlikely) or the "deepEquivalent().parentAnchoredEquivalent()" transformation done inside makeRange could return null for a VisiblePosition that is not null (also unlikely).

So I am pretty sure, but not 100% sure, that the null check I removed was unnecessary. But I also have no reason to take such a risk, so I guess I can add one back in.

I’ll use auto. I think our tastes coincide here — I never quite know when to stop when changing the code in a refactoring project like this.

>> Source/WebCore/accessibility/AccessibilityObject.cpp:1486
>> +    String lineString = plainText(*makeRange(startOfLine(previousVisiblePos), endOfLine(previousVisiblePos)));
> 
> Similarly.

All right. Will do.

[Darin Adler]

Committed r259401: <https://trac.webkit.org/changeset/259401>

[Radar WebKit Bug Importer]

<rdar://problem/61219432>