Summary: | Release assertion failures under Editor::scanSelectionForTelephoneNumbers | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Tim Horton <thorton> | ||||||
Component: | New Bugs | Assignee: | Tim Horton <thorton> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | bdakin, darin, ews-watchlist, megan_gardner, mifenton, rniwa, webkit-bug-importer, wenson_hsieh | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Tim Horton
2021-03-09 22:28:33 PST
Created attachment 422802 [details]
Patch
Comment on attachment 422802 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=422802&action=review > Source/WebCore/editing/Editor.cpp:3705 > + if (selectedRange) { Why not put selectedRange inside here? > Source/WebCore/editing/Editor.cpp:3710 > + extendedRange = extendSelection(*selectedRange, charactersToExtend); > + } > + > + if (extendedRange) { Can't we just nest these if's? Comment on attachment 422802 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=422802&action=review >> Source/WebCore/editing/Editor.cpp:3705 >> + if (selectedRange) { > > Why not put selectedRange inside here? Good point. >> Source/WebCore/editing/Editor.cpp:3710 >> + if (extendedRange) { > > Can't we just nest these if's? Sure, we certainly can! Created attachment 422804 [details]
Patch
Committed r274203: <https://commits.webkit.org/r274203> All reviewed patches have been landed. Closing bug and clearing flags on attachment 422804 [details]. Better. One other thought. To my taste, the real problem here is calling two separate functions, isRange and firstRange, to get the same information. A better design would be to call firstRange and then check if it’s missing or *collapsed* instead of relying on a separate function call to preflight that information. |