[webkit-reviews] review granted: [Bug 130917] Add variant of phone number parsing that use DocumentMarker in the current selection : [Attachment 228189] Patch v3

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Mar 31 14:05:34 PDT 2014


Darin Adler <darin at apple.com> has granted Brady Eidson <beidson at apple.com>'s
request for review:
Bug 130917: Add variant of phone number parsing that use DocumentMarker in the
current selection
https://bugs.webkit.org/show_bug.cgi?id=130917

Attachment 228189: Patch v3 
https://bugs.webkit.org/attachment.cgi?id=228189&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=228189&action=review


> Source/WebCore/dom/DocumentMarker.h:78
>	   // FIXME: iOS has its own dictation marks. iOS should use
OpenSource's.

This comment wording seems a little strange nowadays.

> Source/WebCore/editing/Editor.cpp:3353
> +    // FIXME: This won't work if a phone number spans multiple chunks of
text from the perspective of the TextIterator
> +    // (By a style change, image, line break, etc.)
> +    // For that we'll need to scan the plain text string then do more
complicated things during the TextIterator pass.

For the real thing, numbers that can cross multiple text nodes, we could use
the same model that text searching does, with a rotating window.

> Source/WebCore/editing/Editor.cpp:3355
> +	   // TextIterator currently never returns a Range that spans multiple
Nodes.

Not just currently. This is guaranteed by TextIterator.

> Source/WebCore/editing/Editor.cpp:3383
> +	  
range.ownerDocument().markers().addMarkerToNode(range.startContainer(),
range.startOffset() + scannerPosition + relativeStartPosition,
relativeEndPosition - relativeStartPosition + 1,
DocumentMarker::TelephoneNumber);

This code is assuming that range.startContainer is a text node. We should
probably check that explicitly. The code will do something crazy otherwise.


More information about the webkit-reviews mailing list