[webkit-reviews] review denied: [Bug 132888] Clip Telephone Data Detector Overlay to Scrolling Range : [Attachment 231411] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Brent Fulgham
<bfulgham at webkit.org>'s request for review:
Bug 132888: Clip Telephone Data Detector Overlay to Scrolling Range
https://bugs.webkit.org/show_bug.cgi?id=132888

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

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=231411&action=review


>
Source/WebKit2/WebProcess/WebPage/mac/TelephoneNumberOverlayControllerMac.mm:12
1
> +{
> +    CGContextRef cgContext = graphicsContext.platformContext();
> +
> +    // If this region is enclosed in a non-mainframe scrollable region, we
need to clip.
> +    ContainerNode* enclosedScrollableNode =
findEnclosingScrollableContainer(range.startContainer()->parentNode());
> +    if (!enclosedScrollableNode)
> +	   return;
> +
> +    Frame* frame = nullptr;
> +    if (enclosedScrollableNode->isHTMLDocument())
> +	   frame = static_cast<HTMLDocument*>(enclosedScrollableNode)->frame();

> +    else if (enclosedScrollableNode->isFrameOwnerElement())
> +	   frame =
toHTMLFrameOwnerElement(enclosedScrollableNode)->contentFrame();
> +
> +    if (!frame && !enclosedScrollableNode->isHTMLElement())
> +	   return;
> +    
> +    if (frame && frame->isMainFrame())
> +	   return;
> +
> +    // Must clip:
> +    IntRect clipRect;
> +
> +    if (FrameView* view = frame ? frame->view() : nullptr)
> +	   clipRect = view->windowClipRect();
> +    else if (enclosedScrollableNode->isHTMLElement())
> +	   clipRect = toHTMLElement(enclosedScrollableNode)->clientRect();
> +
> +    clipRect.move(0, -topContentInset);
> +    CGContextClipToRect(cgContext, clipRect);
> +}

I think you should use RenderLayer::childrenClipRect() which we use for
plug-ins and frames in various cases.

I also think this code should live at some lower level, where other things
could make use of it. Makes no sense for this to be in telephone number code.