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

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> 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 179763: Patch
https://bugs.webkit.org/attachment.cgi?id=179763&action=review

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


> Source/WebKit/chromium/src/WebFrameImpl.cpp:1398
> +    VisiblePosition movePosition =
frame()->selection()->selection().visibleStart();

This should be visibleBase();

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1401
> +    IntRect editableRect =
editable->renderer()->enclosingLayer()->absoluteBoundingBox();

This is not right. You need to obtain the bounding box of the roo editable
element.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1405
> +    bool movingUp = point.y < windowMovePosition.y();

This is wrong when we're in a vertical writing mode. r- because of this.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1407
> +	   VisiblePosition newMovePosition = movingUp ?
previousLinePosition(movePosition, 0) : nextLinePosition(movePosition, 0);

Ditto.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1411
> +	   if (movingUp ? point.y > windowNewMovePosition.y() : point.y <
windowNewMovePosition.y())

Ditto.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1414
> +	   if (movingUp ? editableRect.y() > windowNewMovePosition.y():
editableRect.y() + editableRect.height() < windowNewMovePosition.y())

Ditto.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1419
> +    bool movingLeft = point.x < windowMovePosition.x();

Ditto.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1421
> +	   VisiblePosition newMovePosition = movingLeft ?
movePosition.left(true) : movePosition.right(true);

Ditto.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1425
> +	   if (movingLeft ? point.x > windowNewMovePosition.x() : point.x <
windowNewMovePosition.x())

Ditto.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1428
> +	   if (movingLeft ? editableRect.x() > windowNewMovePosition.x():
editableRect.x() + editableRect.width() < windowNewMovePosition.x())

Ditto.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1432
> +    frame()->selection()->moveTo(movePosition, UserTriggered);

You need to call FrameSelection::shouldChangeSelection before calling moveTo.
I'd suggest adding this entire function to FrameSelection instead.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1434
> +    if (viewImpl()->client())
> +	   viewImpl()->client()->didChangeSelection(true);

Why are we directly calling back to didChangeSelection? moveTo calls
FrameSelection::setSelection, which in turn calls
Editor::respondToChangedSelection and then
EditorClient::respondToChangedSelection.