[webkit-reviews] review denied: [Bug 57336] experiment with moving caret by word in visual order : [Attachment 87628] patch w/ layout test

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Xiaomei Ji <xji at chromium.org>'s
request for review:
Bug 57336: experiment with moving caret by word in visual order
https://bugs.webkit.org/show_bug.cgi?id=57336

Attachment 87628: patch w/ layout test
https://bugs.webkit.org/attachment.cgi?id=87628&action=review

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

> Source/WebCore/editing/SelectionController.cpp:451
> +    // FIXME: remove later.

Since we've already added this comment to where we declared
WebKitVisualWordGranularity, I don't think we need to repeat ourselves here.

> Source/WebCore/editing/SelectionController.cpp:493
> +    // FIXME: remove later.

Ditto.

> Source/WebCore/editing/SelectionController.cpp:578
> +    // FIXME: remove later.

Ditto.

> Source/WebCore/editing/SelectionController.cpp:621
> +    // FIXME: remove later.

Ditto.

> Source/WebCore/editing/SelectionController.cpp:668
> +    // FIXME: remove later.

Ditto.

> Source/WebCore/editing/SelectionController.cpp:746
> +    // FIXME: remove later.

Ditto.

> Source/WebCore/editing/VisibleSelection.cpp:383
> +	   // FIXME: remove later.

Ditto.

> Source/WebCore/editing/visible_units.cpp:1160
> +    // For LTR text, previousWordPosition() returns the word break at the
left boundary of a word, 
> +    //		nextWordPosition() returns the word break at the right
boundary of a word. 
> +    // For RTL text, previousWordPosition() returns the word break at the
right boundary of a word,
> +    //		nextWordPosition() returns the word break at the left
boundary of a word.

I don't understand why we need this comment here.  It seems like we want these
comments to be right above previousWordPosition and nextWordPosition instead.
Or, you should explain how these things affect what we do in this function. 
Why explaining what nextWordPosition and previousWordPosition do isn't
particularly helpful here
because this comment appears before any code where this information is
relevant.

> Source/WebCore/editing/visible_units.cpp:1165
> +    // In LTR block, ctrl-arrow moves cursor to the left boundary of words,
> +    // so we should use previousWordPosition() for LTR text, in which
offsets collected from visually right to visually left.
> +    // In RTL block, ctrl-arrow moves cursor to the right boundary of words,

> +    // so we should use previousWordPosiiton() for RTL text, in which,
offsets collected from visually left to  visually right. 

This comment isn't intelligible for me. What does ctrl-arrow mean?  Which arrow
key are we talking about?
You say we should use previousWordPosition() for LTR text but you don't really
explain why we should.
And I can't make a sense of previousWordPosition() "collecting" offsets
visually right to left.

> Source/WebCore/editing/visible_units.cpp:1185
> +	   wordBreak.getInlineBoxAndOffset(boxContainingPreviousWordBreak,
offsetOfWordBreak);

I'd declare boxContainingPreviousWordBreak and offsetOfWordBreak here instead
of at the beginning of the function.  It's okay to have multiple local
variables of the same name as long as they are in separate blocks.

> Source/WebCore/editing/visible_units.cpp:1198
> +	   if (box->direction() == blockDirection)
> +	       wordBreak =
previousWordBreakInBoxInsideBlockWithSameDirectionality(box, wordBreak,
offsetOfWordBreak);

I'm a bit confused here.  If we're looking for a word boundary in the text
direction of block and box and block have the same directionality, shouldn't we
be looking for the next word boundary instead of previous one?

> Source/WebCore/editing/visible_units.cpp:1204
> +    } while (1);	   

Since you're putting 1 in while's condition, I'd use the regular while instead
of do-while.

> Source/WebCore/editing/visible_units.cpp:1221
> +    const InlineBox* adjacentBox = box;
> +    while (adjacentBox) {
> +	   VisiblePosition wordBreak;
> +	   if ((directionToSearch == WebCore::DirectionRight && blockDirection
== LTR) 
> +	       || (directionToSearch == WebCore::DirectionLeft &&
blockDirection == RTL))
> +	       wordBreak = findWordBoundaryInBoxInBlockDirection(adjacentBox,
offset, blockDirection);
> +	   // FIXME: "else" not implemented yet.
> +	   if (!wordBreak.isNull())
> +	       return wordBreak;
> +	   adjacentBox = (directionToSearch == WebCore::DirectionLeft) ?
adjacentBox->nextLeafChild() : adjacentBox->prevLeafChild();
> +	   offset = invalidOffset;
> +    }

Since this code is fairly simple, I'd duplicate code in leftWordBoundary and
rightWordBoundary rather than adding a function that takes directionToSearch.
You can then rewrite this while loop as a simple for loop (leftWordBoundary
case):

static VisiblePosition leftWordBoundary(const InlineBox* box, int offset,
TextDirection blockDirection, const VisiblePosition& visiblePosition)
{
    for (const InlineBox* adjacentBox = box; adjacentBox; adjacentBox =
adjacentBox->prevLeafChild()) {
	if (blockDirection == RTL)
	    wordBreak = findWordBoundaryInBoxInBlockDirection(adjacentBox,
adjacentBox == box ? offset : invalidOffset, blockDirection);
	// FIXME: Implement "else" case.
	if (wordBreak.isNotNull())
	    return wordBreak;
    }
}

> Source/WebCore/editing/visible_units.cpp:1222
> +    return visiblePosition;

I don't understand the point of returning visiblePosition here.  Is this
visiblePosition used in the "else" case?  If not, I'd just return null here and
take care of that in leftWordPosition and rightWordPosition.

> Source/WebCore/editing/visible_units.cpp:1238
> +    // In LTR block, ctrl-arrow moves cursor to the left boundary of words.
> +    // In RTL block, ctrl-arrow moves cursor to the right boundary of words.


I can't make sense of this comment.  I think you should remove it.

> LayoutTests/editing/selection/move-by-word-visually-expected.txt:7
> +"abc def hij opq rst"[0, 4, 8, 12, 16, 19]	 FAIL expected: [4, 8, 12, 16,
19, 19]
> +Move left by one word
> +"abc def hij opq rst"[16, 16, 12, 8, 4, 0]	 FAIL expected: [16, 12, 8, 4,
0, 0]

Very nice output!  I can make sense of outputs now. But the expected result for
the second row doesn't look right.
Since we have "0 4 8 12 16 19|19 16 12 8 4 0" in the title, shouldn't we see
FAIL expected [19, 16, 12, 8, 4, 0] instead?