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

[bugzilla-daemon at webkit.org]

Tim Horton <thorton 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 231434: Patch
https://bugs.webkit.org/attachment.cgi?id=231434&action=review

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


> Source/WebKit2/WebProcess/WebPage/PageOverlay.cpp:242
> +void PageOverlay::clipForRange(FrameView& mainFrameView,
WebCore::GraphicsContext& graphicsContext, const WebCore::Range& range, float
topContentInset)

It seems really weird to me that PageOverlay knows anything about ranges. Does
it need to?

> Source/WebKit2/WebProcess/WebPage/PageOverlayController.cpp:213
> -	   if (overlayAndLayer.key->overlayType() ==
PageOverlay::OverlayType::View || !frame->isMainFrame())
> +	   if (!frame->isMainFrame())

I don't understand this change and it seems bad. You're making View-relative
overlays not repaint on main frame scroll? That's going to destroy
find-in-page, isn't it?

Your changelog says "All overlay types should be informed about scroll events,
not just Views.", but you're only changing what happens if it is a
View-relative overlay.

Also there's no reason why overlays need to always invalidate on scroll. Maybe
we should introduce "needs invalidate on any scroll" and "needs invalidate on
main frame scroll" flags?