Bug 131247 – Show DataDetector UI on scanned phone numbers

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Brady Eidson]

Show DataDetector UI on scanned phone numbers

In radar as <rdar://problem/16379588>

[Brady Eidson]

Created attachment 228627 [details]
Patch v1

[Brady Eidson]

Created attachment 228628 [details]
Patch v2 - Might apply.

[Tim Horton]

Comment on attachment 228627 [details]
Patch v1

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

> Source/WebCore/editing/Editor.cpp:3393
> +        range.ownerDocument().markers().addMarkerToNode(range.startContainer(), startOffset, length, DocumentMarker::TelephoneNumber);

When are these markers removed? (I know you already had them, just curious).

> Source/WebKit2/WebProcess/WebPage/TelephoneNumberController.cpp:76
> +    m_telephoneNumberOverlay = 0;

nullptr

> Source/WebKit2/WebProcess/WebPage/TelephoneNumberController.cpp:88
> +    for (auto& range : m_currentSelectionRanges)
> +        result.append(enclosingIntRect(range->boundingRect()));

I think this will fall down if the phone number is wrapped at the edge of the view?

> Source/WebKit2/WebProcess/WebPage/TelephoneNumberController.h:27
> +#if ENABLE(TELEPHONE_NUMBER_DETECTION)

add space above

> Source/WebKit2/WebProcess/WebPage/TelephoneNumberController.h:30
> +#include "WebPage.h"

is this necessary?

> Source/WebKit2/WebProcess/WebPage/TelephoneNumberController.h:67
> +    virtual void pageOverlayDestroyed(PageOverlay*);
> +    virtual void willMoveToWebPage(PageOverlay*, WebPage*);
> +    virtual void didMoveToWebPage(PageOverlay*, WebPage*);
> +    virtual void drawRect(PageOverlay*, WebCore::GraphicsContext&, const WebCore::IntRect& dirtyRect);
> +    virtual bool mouseEvent(PageOverlay*, const WebMouseEvent&);

are these overrides?

> Source/WebKit2/WebProcess/WebPage/mac/TelephoneNumberControllerMac.mm:27
> +#import "TelephoneNumberController.h"

this class doesn't control telephone numbers.

> Source/WebKit2/WebProcess/WebPage/mac/TelephoneNumberControllerMac.mm:36
> +#ifdef __has_include

I think we decided you don't need the outer ifdef (checking for __has_include).

> Source/WebKit2/WebProcess/WebPage/mac/TelephoneNumberControllerMac.mm:94
> +        highlightBoundingRect.origin.x = 0;
> +        highlightBoundingRect.origin.y = 0;

It might be a good idea to give the modified (highlight-relative) rect a new name.

> Source/WebKit2/WebProcess/WebPage/mac/TelephoneNumberControllerMac.mm:172
> +    m_mouseDownPosition = IntPoint::zero();

Just IntPoint(), I think?

[Brady Eidson]

(In reply to comment #3)
> (From update of attachment 228627 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=228627&action=review
> 
> > Source/WebCore/editing/Editor.cpp:3393
> > +        range.ownerDocument().markers().addMarkerToNode(range.startContainer(), startOffset, length, DocumentMarker::TelephoneNumber);
> 
> When are these markers removed? (I know you already had them, just curious).

Any time the selection changes.

> > Source/WebKit2/WebProcess/WebPage/TelephoneNumberController.cpp:76
> > +    m_telephoneNumberOverlay = 0;
> 
> nullptr

Yup.

> 
> > Source/WebKit2/WebProcess/WebPage/TelephoneNumberController.cpp:88
> > +    for (auto& range : m_currentSelectionRanges)
> > +        result.append(enclosingIntRect(range->boundingRect()));
> 
> I think this will fall down if the phone number is wrapped at the edge of the view?

It probably will.  I'm also not sure what the expectation would be in that case. I think we should file a followup bug.

> 
> > Source/WebKit2/WebProcess/WebPage/TelephoneNumberController.h:27
> > +#if ENABLE(TELEPHONE_NUMBER_DETECTION)
> 
> add space above

K.

> 
> > Source/WebKit2/WebProcess/WebPage/TelephoneNumberController.h:30
> > +#include "WebPage.h"
> 
> is this necessary?

At one point, not anymore.

> 
> > Source/WebKit2/WebProcess/WebPage/TelephoneNumberController.h:67
> > +    virtual void pageOverlayDestroyed(PageOverlay*);
> > +    virtual void willMoveToWebPage(PageOverlay*, WebPage*);
> > +    virtual void didMoveToWebPage(PageOverlay*, WebPage*);
> > +    virtual void drawRect(PageOverlay*, WebCore::GraphicsContext&, const WebCore::IntRect& dirtyRect);
> > +    virtual bool mouseEvent(PageOverlay*, const WebMouseEvent&);
> 
> are these overrides?

Yes could call.
 
> > Source/WebKit2/WebProcess/WebPage/mac/TelephoneNumberControllerMac.mm:27
> > +#import "TelephoneNumberController.h"
> 
> this class doesn't control telephone numbers.

On IRC we agreed to TelephoneNumberOverlayController.

> 
> > Source/WebKit2/WebProcess/WebPage/mac/TelephoneNumberControllerMac.mm:36
> > +#ifdef __has_include
> 
> I think we decided you don't need the outer ifdef (checking for __has_include).

okay.

> 
> > Source/WebKit2/WebProcess/WebPage/mac/TelephoneNumberControllerMac.mm:94
> > +        highlightBoundingRect.origin.x = 0;
> > +        highlightBoundingRect.origin.y = 0;
> 
> It might be a good idea to give the modified (highlight-relative) rect a new name.

Okay.

> 
> > Source/WebKit2/WebProcess/WebPage/mac/TelephoneNumberControllerMac.mm:172
> > +    m_mouseDownPosition = IntPoint::zero();
> 
> Just IntPoint(), I think?

Either.  Wonder why zero() even exists...

[Brady Eidson]

Created attachment 228636 [details]
Patch v3

[WebKit Commit Bot]

Comment on attachment 228636 [details]
Patch v3

Clearing flags on attachment: 228636

Committed r166818: <http://trac.webkit.org/changeset/166818>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Joseph Pecoraro]

Comment on attachment 228636 [details]
Patch v3

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

> Source/WebKit2/WebProcess/WebPage/TelephoneNumberOverlayController.h:83
> +#if PLATFORM(MAC)
> +    Vector<RetainPtr<DDHighlightRef>> m_highlights;
> +    RetainPtr<DDHighlightRef> m_currentMouseDownHighlight;
> +#endif

These are accessed outside of PLATFORM(MAC) specifically PLATFORM(IOS). Are they intended to be PLATFORM(COCOA) or are a lot of these functions that deal with them (drawRect, mouseEvent, etc) PLATFORM(MAC) only right now?

[Joseph Pecoraro]

(In reply to comment #8)
> (From update of attachment 228636 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=228636&action=review
> 
> > Source/WebKit2/WebProcess/WebPage/TelephoneNumberOverlayController.h:83
> > +#if PLATFORM(MAC)
> > +    Vector<RetainPtr<DDHighlightRef>> m_highlights;
> > +    RetainPtr<DDHighlightRef> m_currentMouseDownHighlight;
> > +#endif
> 
> These are accessed outside of PLATFORM(MAC) specifically PLATFORM(IOS). Are they intended to be PLATFORM(COCOA) or are a lot of these functions that deal with them (drawRect, mouseEvent, etc) PLATFORM(MAC) only right now?

Turning the PLATFORM(MAC)s in this file into PLATFORM(COCOA) will get iOS to compile. I will land with that to get the bots continuing to build, but I suspect you will want to include an improved follow-up fix, and either really make these PLATFORM(MAC) or have them all be PLATFORM(COCOA). For example in TelephoneNumberOverlayController::selectedTelephoneNumberRangesChanged I will leave PLATFORM(MAC) as is for now.

[Joseph Pecoraro]

iOS Build Fix: <http://trac.webkit.org/changeset/166821>

You may want to provide a follow-up.