Bug 134784 – Phone number highlights should always be visible if the mouse hovers over

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

[Brady Eidson]

Phone number highlights should always be visible if the mouse hovers over

<rdar://problem/17527476>

[Brady Eidson]

Created attachment 234671 [details]
Patch v1

[Tim Horton]

Comment on attachment 234671 [details]
Patch v1 

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

[Brady Eidson]

(In reply to comment #2)
> (From update of attachment 234671 [details])
> 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?

Unfortunately I couldn't come up with one, otherwise I would've used it already  =/

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

I did not know you could do that and I hate it.  But I made the change in this patch.

> > 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"?

Good change.

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

Agreed.

> > Source/WebKit2/WebProcess/WebPage/mac/ServicesOverlayController.mm:398
> > +            CGRect cgRect = (CGRect)rect;
> 
> do you need this cast? I thought IntRect->CGRect was implicit.

Nope!  Had it before for whatever reason, carried it over here...  it's not needed.

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

Nope, the new small overlays came in after this was originally written.  It *should* use the smaller ones, and it should use them in WebCore...  <wink wink nudge nudge>

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

Yes, it is uninstalled/destroyed before the page goes away.

[Brady Eidson]

http://trac.webkit.org/changeset/170966

[Tim Horton]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 234671 [details] [details])
> > 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?
> 
> Unfortunately I couldn't come up with one, otherwise I would've used it already  =/

Yeah, there's a reason I didn't make a suggestion.

> > > 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.
> 
> Nope, the new small overlays came in after this was originally written.  It *should* use the smaller ones, and it should use them in WebCore...  <wink wink nudge nudge>

Someday!

> > > 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
> 
> Yes, it is uninstalled/destroyed before the page goes away.

Where? I see how it is uninstalled *when* the page goes away, but that means that if you have a single selection/telephone number highlight, we have the (nonopaque) overlay tiles on top of the content until the page goes away, even if we're painting nothing into them. Expensive! (2x the memory, no opaque compositing optimizations, etc.)

[Brady Eidson]

(In reply to comment #5)
> > > > 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
> > 
> > Yes, it is uninstalled/destroyed before the page goes away.
> 
> Where? I see how it is uninstalled *when* the page goes away, but that means that if you have a single selection/telephone number highlight, we have the (nonopaque) overlay tiles on top of the content until the page goes away, even if we're painting nothing into them. Expensive! (2x the memory, no opaque compositing optimizations, etc.)

Ugh.  Either the old TelephoneNumber overlay or the old Selection overly or both did this...  but you are correct, we don't do it now.

Filed https://bugs.webkit.org/show_bug.cgi?id=134803