| Summary: | Remove all uses of live ranges from TextIterator | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Darin Adler <darin> | ||||||||||||
| Component: | HTML Editing | Assignee: | Darin Adler <darin> | ||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||
| Severity: | Normal | CC: | aboxhall, apinheiro, cdumez, cfleizach, cmarcelo, dmazzoni, dpino, esprehn+autocc, ews-watchlist, gyuyoung.kim, jcraig, jdiggs, kangil.han, koivisto, mifenton, samuel_white, webkit-bug-importer, wenson_hsieh | ||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||
| Hardware: | All | ||||||||||||||
| OS: | All | ||||||||||||||
| See Also: | https://bugs.webkit.org/show_bug.cgi?id=209966 | ||||||||||||||
| Bug Depends on: | 209966 | ||||||||||||||
| Bug Blocks: | |||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Darin Adler
2020-03-29 21:43:05 PDT
Created attachment 394885 [details]
Patch
Created attachment 394886 [details]
Patch
Created attachment 395221 [details]
Patch
Created attachment 395229 [details]
Patch
Created attachment 395241 [details]
Patch
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. Antti, would you be willing to review this one? All the tests are passing. 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 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. Committed r259401: <https://trac.webkit.org/changeset/259401> |