[webkit-reviews] review denied: [Bug 92513] [chromium] Upstream Android changes to WebFrameImpl::selectRange : [Attachment 156931] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Iain Merrick <husky at google.com>'s
request for review:
Bug 92513: [chromium] Upstream Android changes to WebFrameImpl::selectRange
https://bugs.webkit.org/show_bug.cgi?id=92513

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

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=156931&action=review


> Source/WebCore/page/EventHandler.cpp:760
> -static VisiblePosition selectionExtentRespectingEditingBoundary(const
VisibleSelection& selection, const LayoutPoint& localPoint, Node* targetNode)
> +VisiblePosition EventHandler::visiblePositionRespectingEditingBoundary(const
HitTestResult& hitTestResult)

Why are we renaming this function? This function is about adjusting extent, not
either base or extent.

> Source/WebCore/page/EventHandler.cpp:762
> +    LayoutPoint layoutPoint = hitTestResult.localPoint();

Why are we renaming this variable to layoutPoint? That doesn't explain
anything.
It's like naming a variable int or boolean.

> Source/WebCore/page/EventHandler.cpp:3714
> +    VisiblePosition startPosition =
visiblePositionRespectingEditingBoundary(hitTestStart);
> +    VisiblePosition endPosition =
visiblePositionRespectingEditingBoundary(hitTestEnd);

This is not right. Either end point needs to be base, and that's the one we
should be adjusting, not both.

> Source/WebCore/page/EventHandler.cpp:3716
> +    VisibleSelection newSelection(startPosition, endPosition);

We need to know which point is base & extent here to create a selection
properly.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1461
> +    frame()->eventHandler()->handleSelectRange(IntPoint(start.x, start.y),
IntPoint(end.x, end.y));

We should definitely not be calling into eventHandler here since we're not
handling an event.