Bug 130917 - Add variant of phone number parsing that use DocumentMarker in the current selection
Summary: Add variant of phone number parsing that use DocumentMarker in the current se...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-03-28 16:41 PDT by Brady Eidson
Modified: 2014-03-31 14:54 PDT (History)
1 user (show)

See Also:


Attachments
Patch v1 (14.51 KB, patch)
2014-03-28 16:48 PDT, Brady Eidson
beidson: review-
Details | Formatted Diff | Diff
Patch v2 (13.93 KB, patch)
2014-03-31 11:51 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch v3 (13.98 KB, patch)
2014-03-31 12:45 PDT, Brady Eidson
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2014-03-28 16:41:15 PDT
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>
Comment 1 Brady Eidson 2014-03-28 16:48:32 PDT
Created attachment 228091 [details]
Patch v1
Comment 2 WebKit Commit Bot 2014-03-28 16:49:40 PDT
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.
Comment 3 Tim Horton 2014-03-28 17:00:47 PDT
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?
Comment 4 Sam Weinig 2014-03-28 19:53:30 PDT
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.
Comment 5 Brady Eidson 2014-03-28 22:13:21 PDT
(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.
Comment 6 Brady Eidson 2014-03-28 22:15:55 PDT
(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?
Comment 7 Brady Eidson 2014-03-31 11:51:06 PDT
Created attachment 228184 [details]
Patch v2
Comment 8 Brady Eidson 2014-03-31 12:45:16 PDT
Created attachment 228189 [details]
Patch v3
Comment 9 Darin Adler 2014-03-31 14:05:54 PDT
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.
Comment 10 Brady Eidson 2014-03-31 14:16:35 PDT
(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.
Comment 11 Brady Eidson 2014-03-31 14:17:08 PDT
> > > Source/WebCore/editing/Editor.cpp:3383
> Good call, wild.

s/wild/willdo/ (DYAC!)
Comment 12 Brady Eidson 2014-03-31 14:54:51 PDT
https://trac.webkit.org/changeset/166532