RESOLVED FIXED 135220
REGRESSION (r171376): Sometimes we detect less than the whole phone number
https://bugs.webkit.org/show_bug.cgi?id=135220
Summary REGRESSION (r171376): Sometimes we detect less than the whole phone number
Tim Horton
Reported 2014-07-23 16:50:07 PDT
This is happening because the detection code will happily say that (###) ###-### is a real phone number (it probably is, somewhere), and our approach of using TextIterator and adjusting the ranges sometimes results in a fragment of a number being matched against first — before the whole number is passed in. The code in scanSelectionForTelephoneNumbers doesn't really know how to merge these, and we have no way to predict what the detection code will do, so we should instead hand *the whole extended selection* in one chunk to the detection code, so it can eagerly detect all of the phone numbers. Doing this made it clear that the code to extend the selection was quite broken in the case of a directional selection (if it was a selection from right to left, the extension code would actually squish the selection instead of expanding it). I haven't found any cases that this makes *worse*, and it is significantly simpler. <rdar://problem/17783423>
Attachments
patch (5.88 KB, patch)
2014-07-23 16:54 PDT, Tim Horton
no flags
patch (5.84 KB, patch)
2014-07-23 17:08 PDT, Tim Horton
beidson: review+
Tim Horton
Comment 1 2014-07-23 16:54:48 PDT
Tim Horton
Comment 2 2014-07-23 17:08:43 PDT
Brady Eidson
Comment 3 2014-07-23 17:12:42 PDT
This sounds great, and is probably what we should've done in the first place.
Brady Eidson
Comment 4 2014-07-23 17:14:08 PDT
Comment on attachment 235393 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=235393&action=review > Source/WebCore/editing/Editor.cpp:3438 > + Really necessary? Paragraphing seemed okay before.
Tim Horton
Comment 5 2014-07-23 17:22:22 PDT
(In reply to comment #4) > (From update of attachment 235393 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=235393&action=review > > > Source/WebCore/editing/Editor.cpp:3438 > > + > > Really necessary? Paragraphing seemed okay before. Not necessary! Just left over from some debugging logging that I failed to remove entirely. Thanks for your review!
Tim Horton
Comment 6 2014-07-23 17:24:47 PDT
(In reply to comment #5) > (In reply to comment #4) > > (From update of attachment 235393 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=235393&action=review > > > > > Source/WebCore/editing/Editor.cpp:3438 > > > + > > > > Really necessary? Paragraphing seemed okay before. > > Not necessary! Just left over from some debugging logging that I failed to remove entirely. Though somehow I managed to land it anyway :| > Thanks for your review!
Tim Horton
Comment 7 2014-07-23 17:25:13 PDT
Note You need to log in before you can comment on or make changes to this bug.