Bug 131247 - Show DataDetector UI on scanned phone numbers
Summary: Show DataDetector UI on scanned phone numbers
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac All
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-04-04 15:46 PDT by Brady Eidson
Modified: 2014-04-04 20:05 PDT (History)
4 users (show)

See Also:


Attachments
Patch v1 (37.21 KB, patch)
2014-04-04 15:52 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch v2 - Might apply. (37.11 KB, patch)
2014-04-04 15:59 PDT, Brady Eidson
thorton: review+
Details | Formatted Diff | Diff
Patch v3 (35.65 KB, patch)
2014-04-04 17:17 PDT, Brady Eidson
no flags 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-04-04 15:46:36 PDT
Show DataDetector UI on scanned phone numbers

In radar as <rdar://problem/16379588>
Comment 1 Brady Eidson 2014-04-04 15:52:53 PDT
Created attachment 228627 [details]
Patch v1
Comment 2 Brady Eidson 2014-04-04 15:59:29 PDT
Created attachment 228628 [details]
Patch v2 - Might apply.
Comment 3 Tim Horton 2014-04-04 16:05:44 PDT
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?
Comment 4 Brady Eidson 2014-04-04 16:20:27 PDT
(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...
Comment 5 Brady Eidson 2014-04-04 17:17:29 PDT
Created attachment 228636 [details]
Patch v3
Comment 6 WebKit Commit Bot 2014-04-04 18:10:38 PDT
Comment on attachment 228636 [details]
Patch v3

Clearing flags on attachment: 228636

Committed r166818: <http://trac.webkit.org/changeset/166818>
Comment 7 WebKit Commit Bot 2014-04-04 18:10:41 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Joseph Pecoraro 2014-04-04 19:44:22 PDT
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?
Comment 9 Joseph Pecoraro 2014-04-04 19:53:34 PDT
(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.
Comment 10 Joseph Pecoraro 2014-04-04 20:05:09 PDT
iOS Build Fix: <http://trac.webkit.org/changeset/166821>

You may want to provide a follow-up.