[webkit-reviews] review granted: [Bug 224837] [iOS] Text selection in image overlays should not be limited to rectilinear quads : [Attachment 426609] Depends on #224820

[bugzilla-daemon at webkit.org]

Tim Horton <thorton at apple.com> has granted Wenson Hsieh
<wenson_hsieh at apple.com>'s request for review:
Bug 224837: [iOS] Text selection in image overlays should not be limited to
rectilinear quads
https://bugs.webkit.org/show_bug.cgi?id=224837

Attachment 426609: Depends on #224820

https://bugs.webkit.org/attachment.cgi?id=426609&action=review




--- Comment #2 from Tim Horton <thorton at apple.com> ---
Comment on attachment 426609
  --> https://bugs.webkit.org/attachment.cgi?id=426609
Depends on #224820

View in context: https://bugs.webkit.org/attachment.cgi?id=426609&action=review

> Source/WebCore/rendering/RenderLineBreak.cpp:235
> +    auto behavior = HTMLElement::isImageOverlayText(element()) ?
SelectionRenderingBehavior::UseIndividualQuads :
SelectionRenderingBehavior::CoalesceBoundingRects;

It's a bit unfortunate that isImageOverlayText is infecting something like
RenderLineBreak. Can we maybe hide it in a SelectionGeometry helper or
something?

> Source/WebCore/rendering/RenderObject.cpp:740
> +    auto behavior = HTMLElement::isImageOverlayText(node()) ?
SelectionRenderingBehavior::UseIndividualQuads :
SelectionRenderingBehavior::CoalesceBoundingRects;

Ditto

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:4808
> -static NSArray<WKTextSelectionRect *> *wkTextSelectionRects(const
Vector<WebCore::SelectionGeometry>& rects)
> +static NSArray<WKTextSelectionRect *> *wkTextSelectionRects(const
Vector<WebCore::SelectionGeometry>& rects, CGFloat scaleFactor)

While you're here... maybe drop the `wk` prefix?

> Source/WebKit/UIProcess/ios/WKTextSelectionRect.mm:114
> +- (WKTextSelectionRectCustomHandleInfo *)_customHandleInfo

Returning a random unrelated type here where this expects a
UITextSelectionRectCustomHandleInfo is pretty sketchy, even if it's "OK". Can
WKTextSelectionRectCustomHandleInfo be a subclass of
WKTextSelectionRectCustomHandleInfo like WKTextSelectionRect is a subclass of
UITextSelectionRect?

> Source/WebKit/UIProcess/ios/WKTextSelectionRect.mm:121
> +    return [[[WKTextSelectionRectCustomHandleInfo alloc]
initWithFloatQuad:scaledQuad] autorelease];

See https://commits.webkit.org/r273062; I think this should be
adoptNS().autorelease() for future-ARC-happiness

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:265
> +	   convertedQuad.setP1(view.contentsToRootView(convertedQuad.p1()));

There are two other instances of this code (InspectorTimelineAgent and
sendTapHighlightForNode), maybe it's time for ScrollView/FrameView to get a
FloatQuad version of contentsToRootView??