Bug 134784 - Phone number highlights should always be visible if the mouse hovers over
Summary: Phone number highlights should always be visible if the mouse hovers over
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-07-09 17:13 PDT by Brady Eidson
Modified: 2014-07-10 09:45 PDT (History)
1 user (show)

See Also:


Attachments
Patch v1 (25.64 KB, patch)
2014-07-09 17:19 PDT, Brady Eidson
thorton: 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-07-09 17:13:39 PDT
Phone number highlights should always be visible if the mouse hovers over

<rdar://problem/17527476>
Comment 1 Brady Eidson 2014-07-09 17:19:28 PDT
Created attachment 234671 [details]
Patch v1
Comment 2 Tim Horton 2014-07-10 09:02:09 PDT
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
Comment 3 Brady Eidson 2014-07-10 09:22:37 PDT
(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.
Comment 4 Brady Eidson 2014-07-10 09:22:45 PDT
http://trac.webkit.org/changeset/170966
Comment 5 Tim Horton 2014-07-10 09:32:24 PDT
(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.)
Comment 6 Brady Eidson 2014-07-10 09:45:41 PDT
(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