[webkit-reviews] review denied: [Bug 92513] [chromium] Upstream Android changes to WebFrameImpl::selectRange : [Attachment 155547] 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 155547: Patch
https://bugs.webkit.org/attachment.cgi?id=155547&action=review

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


> Source/WebKit/chromium/src/WebFrameImpl.cpp:1460
> +    if (startPosition.isNull() || endPosition.isNull() || startPosition ==
endPosition)

We never set a collapsed selection by this function? Why?

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1472
> +    if (frame()->selection()->isContentEditable()) {
> +	   startPosition =
frame()->selection()->selection().visibleStart().honorEditingBoundaryAtOrAfter(
startPosition);
> +	   endPosition =
frame()->selection()->selection().visibleEnd().honorEditingBoundaryAtOrBefore(e
ndPosition);
> +	   if (startPosition.isNull() || endPosition.isNull())
> +	       return;
> +    }

This is done in setSelection.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1479
> +    if (startPosition != endPosition && isEndOfBlock(startPosition)) {
> +	   VisiblePosition nextStartPosition(startPosition.next());
> +	   if (!nextStartPosition.isNull())
> +	       startPosition = nextStartPosition;
> +    }

Why are we doing this?

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1535
> +	       if (currentCaretRect.x() > point.x) {
> +		   // Cursor should move left unless that would result in a
line change or would result in the cursor
> +		   // moving past the selection point.
> +		   movePosition = currentPosition.previous();
> +		   shouldMove =
frame()->view()->contentsToWindow(movePosition.absoluteCaretBounds()).x() >
point.x;
> +		   moveDirection = DirectionLeft;
> +	       } else if (currentCaretRect.x() < point.x) {
> +		   // Cursor should move right unless that would result in a
line change or would result in the cursor
> +		   // moving past the selection point.
> +		   movePosition = currentPosition.next();
> +		   shouldMove =
frame()->view()->contentsToWindow(movePosition.absoluteCaretBounds()).x() <
point.x;
> +		   moveDirection = DirectionRight;
> +	       }
> +
> +	       if (shouldMove && inSameLine(currentPosition, movePosition))
> +		   frame()->selection()->modify(FrameSelection::AlterationMove,
moveDirection, CharacterGranularity, UserTriggered);
> +
> +	       shouldMove = false;
> +	       if (currentCaretRect.y() > point.y) {
> +		   // Cursor should move up unless that would not result in a
line change
> +		   // as that means it's already at the top.
> +		   shouldMove = !inSameLine(currentPosition,
previousLinePosition(currentPosition, 0));
> +		   moveDirection = DirectionBackward;
> +	       } else if (currentCaretRect.y() < point.y) {
> +		   // Cursor should move down unless that would not result in a
line change
> +		   // as that means it's already at the bottom.
> +		   shouldMove = !inSameLine(currentPosition,
nextLinePosition(currentPosition, 0));
> +		   moveDirection = DirectionForward;
> +	       }

What? Why do we need all of this code. This clearly doesn't work at all for
bidirectional text.
See what EventHandler::handleMousePressEventSingleClick and other functions in
EventHandler.cpp do.
We should be re-using the code there.