Bug 130917 – Add variant of phone number parsing that use DocumentMarker in the current selection

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Brady Eidson]

Convert phone number parsing to use DocumentMarkers rather than DOM mutation.

For now we'll also only scan in selections instead of doing detection during parsing, to avoid a PLT regression.

<rdar://problem/16379566>

[Brady Eidson]

Created attachment 228091 [details]
Patch v1

[WebKit Commit Bot]

Attachment 228091 [details] did not pass style-queue:


ERROR: Source/WebCore/html/parser/HTMLTreeBuilder.cpp:2370:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/WebCore/html/parser/HTMLTreeBuilder.cpp:2371:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/WebCore/html/parser/HTMLTreeBuilder.cpp:2380:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 3 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tim Horton]

Comment on attachment 228091 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=228091&action=review

> Source/WebCore/editing/Editor.cpp:3319
> +#if ENABLE(TELEPHONE_NUMBER_DETECTION)

Did this ENABLE flag already exist?

> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:2370
> +//static inline bool disallowTelephoneNumberParsing(const Node& node)

We don't usually leave commented out code in the tree.

> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:2388
> +    UNUSED_PARAM(node);
> +    return false;

Please make sure this won't break iOS.

> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:2390
> +//    const ContainerNode* currentNode = &node;

Ditto

> Source/WebCore/rendering/InlineTextBox.cpp:1254
> +    // FIXME: This is placeholder UI for development of the feature.

Not a useful comment in the context of the open source project.

> Source/WebCore/rendering/InlineTextBox.cpp:1273
> +    outline.addRoundedRect(markerRect, FloatSize(4, 4));
> +    ctx->setFillColor(Color(150, 150, 150, 255), ColorSpaceDeviceRGB);

Maybe pull constants out so they can have names?

[Sam Weinig]

Comment on attachment 228091 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=228091&action=review

> Source/WebCore/editing/Editor.cpp:3344
> +    clearDataDetectedTelephoneNumbers();

Will this also get called when the selection changes to "no selection"?

> Source/WebCore/editing/Editor.h:474
> +    void startTelephoneNumberScan();

I don't think startTelephoneNumberScan() is a great name. How about scanSelectionForTelephoneNumbers().

>> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:2388
>> +    return false;
> 
> Please make sure this won't break iOS.

You should keep all this code in, as it it is still need on iOS.

[Brady Eidson]

(In reply to comment #4)
> >> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:2388
> >> +    return false;
> > 
> > Please make sure this won't break iOS.
> 
> You should keep all this code in, as it it is still need on iOS.

This is why I pulled the patch from review myself, as I had misunderstood the split.  New patch will come tomorrow resolving that and other feedback.

[Brady Eidson]

(In reply to comment #3)
> (From update of attachment 228091 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=228091&action=review
> 
> > Source/WebCore/editing/Editor.cpp:3319
> > +#if ENABLE(TELEPHONE_NUMBER_DETECTION)
> 
> Did this ENABLE flag already exist?

Yes, and is used to guard this feature throughout WebCore.

> > Source/WebCore/rendering/InlineTextBox.cpp:1254
> > +    // FIXME: This is placeholder UI for development of the feature.
> 
> Not a useful comment in the context of the open source project.

Features are developed in the open source project often (always) with intermittent states of incompletion, and I've found such comments to be useful in the past.

> > Source/WebCore/rendering/InlineTextBox.cpp:1273
> > +    outline.addRoundedRect(markerRect, FloatSize(4, 4));
> > +    ctx->setFillColor(Color(150, 150, 150, 255), ColorSpaceDeviceRGB);
> 
> Maybe pull constants out so they can have names?

I suppose one reason I hadn't considered that is because "This is placeholder UI" and is not long for this world?

[Brady Eidson]

Created attachment 228184 [details]
Patch v2

[Brady Eidson]

Created attachment 228189 [details]
Patch v3

[Darin Adler]

Comment on attachment 228189 [details]
Patch v3 

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.

[Brady Eidson]

(In reply to comment #9)
> (From update of attachment 228189 [details])
> 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.

Indeed, will at least update it.

> > Source/WebCore/editing/Editor.cpp:3355
> > +        // TextIterator currently never returns a Range that spans multiple Nodes.
> 
> Not just currently. This is guaranteed by TextIterator.

That's good to know.  Nobody else I'd asked was able to state it as fact like you just did.  :)

> 
> > 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.

Good call, wild.

[Brady Eidson]

> > > Source/WebCore/editing/Editor.cpp:3383
> Good call, wild.

s/wild/willdo/ (DYAC!)

[Brady Eidson]

https://trac.webkit.org/changeset/166532