Bug 209723 - Remove all uses of live ranges from TextIterator
Summary: Remove all uses of live ranges from TextIterator
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: 209966
Blocks:
  Show dependency tree / graph
 
Reported: 2020-03-29 21:43 PDT by Darin Adler
Modified: 2020-04-03 09:06 PDT (History)
18 users (show)

See Also:

Attachments
Patch (76.79 KB, patch)
2020-03-29 21:43 PDT, Darin Adler 		no flags Details | Formatted Diff | Diff
Patch (80.78 KB, patch)
2020-03-29 22:02 PDT, Darin Adler 		no flags Details | Formatted Diff | Diff
Patch (90.55 KB, patch)
2020-04-01 16:20 PDT, Darin Adler 		no flags Details | Formatted Diff | Diff
Patch (87.84 KB, patch)
2020-04-01 17:40 PDT, Darin Adler 		no flags Details | Formatted Diff | Diff
Patch (101.37 KB, patch)
2020-04-01 21:22 PDT, Darin Adler 		koivisto: review+
Details | Formatted Diff | Diff
Show Obsolete (4) 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-29 21:43:05 PDT 
Remove all uses of live ranges from TextIterator
Comment 1 Darin Adler 2020-03-29 21:43:59 PDT Comment hidden (obsolete)
Comment 2 Darin Adler 2020-03-29 22:02:19 PDT Comment hidden (obsolete)
Comment 3 Darin Adler 2020-04-01 16:20:52 PDT Comment hidden (obsolete)
Comment 4 Darin Adler 2020-04-01 17:40:19 PDT Comment hidden (obsolete)
Comment 5 Darin Adler 2020-04-01 21:22:17 PDT 
Created attachment 395241 [details]
Patch
Comment 6 Darin Adler 2020-04-01 21:23:45 PDT 
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.
Comment 7 Darin Adler 2020-04-02 09:04:36 PDT 
Antti, would you be willing to review this one? All the tests are passing.
Comment 8 Antti Koivisto 2020-04-02 10:24:27 PDT 
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.
Comment 9 Darin Adler 2020-04-02 10:40:27 PDT 
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.
Comment 10 Darin Adler 2020-04-02 11:57:21 PDT 
Committed r259401: <https://trac.webkit.org/changeset/259401>
Comment 11 Radar WebKit Bug Importer 2020-04-02 11:58:14 PDT 
<rdar://problem/61219432>