[webkit-reviews] review granted: [Bug 134784] Phone number highlights should always be visible if the mouse hovers over : [Attachment 234671] Patch v1

[bugzilla-daemon at webkit.org]

Tim Horton <thorton at apple.com> has granted Brady Eidson <beidson at apple.com>'s
request for review:
Bug 134784: Phone number highlights should always be visible if the mouse
hovers over
https://bugs.webkit.org/show_bug.cgi?id=134784

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

------- Additional Comments from Tim Horton <thorton at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=234671&action=review


> Source/WebKit2/ChangeLog:15
> +	   The exception is establishHoveredTelephoneHighlight which gets a
more detailed explanation below.

Maybe it can have a better name, then?

> Source/WebKit2/WebProcess/WebPage/ServicesOverlayController.h:48
> +    TelephoneNumberData(RetainPtr<DDHighlightRef> theHighlight,
PassRefPtr<WebCore::Range> theRange)

These "the"s are weird, and you can actually have the names match the members,
that works fine, if a little weird.

> Source/WebKit2/WebProcess/WebPage/mac/ServicesOverlayController.mm:265
> +    if (maybeDrawTelephoneNumberHighlight(graphicsContext, dirtyRect))

It's weird that the call sites of these two "maybe" functions are so vague
about when it will happen. Maybe "drawTelephoneNumberHighlightIfVisible"?

> Source/WebKit2/WebProcess/WebPage/mac/ServicesOverlayController.mm:378
> +void ServicesOverlayController::establishHoveredTelephoneHighlight(Boolean&
onButton)

It's really weird that you're using Boolean internally to this extent.

> Source/WebKit2/WebProcess/WebPage/mac/ServicesOverlayController.mm:398
> +	       CGRect cgRect = (CGRect)rect;

do you need this cast? I thought IntRect->CGRect was implicit.

> Source/WebKit2/WebProcess/WebPage/mac/ServicesOverlayController.mm:408
> +	   m_servicesOverlay->setNeedsDisplay();

someday we should tighten up these repaints (or use a smaller overlay, which I
thought we were already doing but see no indication of). not today.

> Source/WebKit2/WebProcess/WebPage/mac/ServicesOverlayController.mm:522
> +	   ASSERT_NOT_REACHED();

once you create the overlay, is it ever uninstalled/destroyed before the page
goes away?! if not, that seems ... bad