[webkit-reviews] review denied: [Bug 105189] Add function to move caret selection towards a point : [Attachment 180067] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Chris Hopman
<cjhopman at chromium.org>'s request for review:
Bug 105189: Add function to move caret selection towards a point
https://bugs.webkit.org/show_bug.cgi?id=105189

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

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


> Source/WebCore/rendering/RenderLayer.cpp:2108
> +	   LayoutRect unscaledRect = rect;
> +	   Frame* frame = renderer()->frame();
> +	   if (frame)
> +	       unscaledRect.scale(1 / frame->page()->pageScaleFactor());
> +	   LayoutRect exposeRect = LayoutRect(unscaledRect.x() +
scrollXOffset(), unscaledRect.y() + scrollYOffset(), unscaledRect.width(),
unscaledRect.height());

Please do this in a separate patch with its own testcase.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1399
> +    Element* editable = frame()->selection()->rootEditableElement();
> +    IntPoint contentsPoint =
frame()->view()->windowToContents(IntPoint(point));
> +    float pageScaleFactor = frame()->page()->pageScaleFactor();
> +    contentsPoint.scale(1 / pageScaleFactor, 1 / pageScaleFactor);
> +    LayoutPoint
localPoint(editable->renderer()->absoluteToLocal(contentsPoint));
> +    VisiblePosition position =
editable->renderer()->positionForPoint(localPoint);
> +    frame()->selection()->moveTo(position, UserTriggered);

YOu should test this in a subframe with page scale applied. I'm not sure
whether windowToContents() on a subframe applies the page scale. Also, if
you're in a subframe, pageScale doesn't apply. frameScaleFactor() is more
useful here.

It also looks like you assume windowToContents() gives you an "absolute" point
in that frame (relative to the top left of the document, even when scrolled),
since you then call absoluteToLocal() on it.

A good test here would be a zoomed page with both the root and the subframe
scrolled. Ideally you'd also have CSS transforms on an ancestor of the frame,
and inside the frame.