[webkit-reviews] review denied: [Bug 57336] experiment with moving caret by word in visual order : [Attachment 87322] 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 87322: patch w/ layout test
https://bugs.webkit.org/attachment.cgi?id=87322&action=review

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

Great! This patch is much more reviewable.

> Source/WebCore/editing/TextGranularity.h:44
> +    // caret by wordGranularity. After all patches in, it should be removed.


Nit: Once all patches are landed

> Source/WebCore/editing/visible_units.cpp:1237
> +    bool first = previousWordBreak.isNull(); // Whether this is to collect
the first word break in the box.

Instead of adding a comment here, let us instead have:
bool hasSeenWordBreakInThisBox = previousWordBreak.isNotNull();

> Source/WebCore/editing/visible_units.cpp:1238
> +    InlineBox* boxOfWordBreak;

Nit: should this be named as boxContainingPreviousWordBreak?

> Source/WebCore/editing/visible_units.cpp:1240
> +    if (box->direction() == blockDirection) {

This function should probably named as nextWordBreakInBox or
previousWordBreakInBoxInVisualOrder.

It also seems like we should split this function into two versions the one for
box->direction() == blockDirection and (box->direction() != blockDirection.
They can be named as:
previousWordBreakInBoxInsideBlockWithSameDirectionality
nextWordBreakInBoxInsideBlockWithOppositeDirectionality

> Source/WebCore/editing/visible_units.cpp:1243
> +	   wordBreak = first ?
VisiblePosition(Position(box->renderer()->node(), box->caretMaxOffset(),
Position::PositionIsOffsetInAnchor), VP_DEFAULT_AFFINITY) : previousWordBreak;

You don't need to explicitly call VisiblePosition's constructor.  it'll
automatically do it.  So you can do:
wordBreak = first ? Position(box->renderer()->node(), box->caretMaxOffset(),
Position::PositionIsOffsetInAnchor) : previousWordBreak;

> Source/WebCore/editing/visible_units.cpp:1245
> +	   if (first && box->nextLeafChild()) {

It seems that this condition should be dependent on the value of
directionality.  It doesn't make sense that we check the same box for both LTR
and RTL cases.

> Source/WebCore/editing/visible_units.cpp:1248
> +	       // when cursor is at the right of "opq", its previous word
boundary's offset returned from previousWordPosition() is 15, which is not in
the same box as "opq",

Nit: immediately to the right of "opq".
Nit: offset from previousWordPosition() is immediately to the right of "hij ".

> Source/WebCore/editing/visible_units.cpp:1262
> +	   last = true;
> +	   return VisiblePosition();

For this version at least, it seems simpler to get rid of "last" argument and
use the null visible position to mean the last.

> Source/WebCore/editing/visible_units.cpp:1264
> +    // FIXME in next version.

FIXME implies "next version".  We should explain what need to be implemented. 
I guess:
FIXME: Implement the case when the box direction is not equal to the block
direction
would work here.

> Source/WebCore/editing/visible_units.cpp:1277
> +// Get the leftmost or rightmost word bounary in a box.
> +// 'box' is the box to start search.
> +// 'offset' is the node offset of the current caret.
> +// When 'offset' is valid (not equal to -1), the word boundary returned
should be different from the word boundary starts at 'offset'.
> +// 'boundaryDirection' indicates the visual direction (left or right) to
search for
> +// word boundary.
> +// 'boundaryDirection' == DirectionRight means search rightmost word
boundary in box.
> +// 'boundaryDirection' == DirectionLeft means search leftmost word boundary
in box.
> +// 'blockDirection' indicates the directionality of the current block.

No need for these comments.

> Source/WebCore/editing/visible_units.cpp:1291
> +	   VisiblePosition wordBreak = collectOneWordBreakInBox(box,
blockDirection, VisiblePosition(), offsetOfWordBreak, last);
> +	   while (!last) {
> +	       if (offset == -1 || offsetOfWordBreak != offset)
> +		   return wordBreak;
> +	       wordBreak = collectOneWordBreakInBox(box, blockDirection,
wordBreak, offsetOfWordBreak, last);
> +	   }

This can be written as a do-while as in:
VisiblePosition wordBreak;
do {
    wordBreak = collectOneWordBreakInBox(box, blockDirection, wordBreak,
offsetOfWordBreak, last);
    if (offset == -1 || offsetOfWordBreak != offset)
	return wordBreak;
} while (!last);

> Source/WebCore/editing/visible_units.cpp:1293
> +	   if (!wordBreak.isNull() && (offset == -1 || offsetOfWordBreak !=
offset))
> +	       return wordBreak;

This is dead code for this version.  Please remove.

> Source/WebCore/editing/visible_units.cpp:1299
> +// Search word boundary in visually adjacent (previous or next) boxes.

This repeats the function name. Please remove it.

> Source/WebCore/editing/visible_units.cpp:1301
> +// 'box' is the box to start search.
> +// 'offset' is the node offset of the current caret.

No need. Please get rid of it.

> Source/WebCore/editing/visible_units.cpp:1302
> +// When 'offset' is valid (not equal to -1), the word boundary returned
should be different from the word boundary starts at 'offset'.

We should define a value as in #define INVALID_OFFSET -1 so that we don't need
to add this comment.

> Source/WebCore/editing/visible_units.cpp:1308
> +// 'boundaryDirection' indicates the visual direction (left or right) to
search for
> +// word boundary.
> +// 'boundaryDirection' == DirectionRight means search
rightmostWordBoundaryInPreviousBoxes.
> +// 'boundaryDirection' == DirectionLeft means search
leftmostWordBoundaryInNextBoxes.
> +// 'blockDirection' indicates the directionality of the current block.
> +// 'visiblePosition' is the visible position of current caret.

I don't think these comments are necessary.

> Source/WebCore/editing/visible_units.cpp:1309
> +static VisiblePosition wordBoundaryInAdjacentBoxes(const InlineBox* box, int
offset, WebCore::SelectionDirection boundaryDirection, TextDirection
blockDirection, const VisiblePosition& visiblePosition)

boundaryDirection might be a bit confusing?  Maybe directionToSearch?

> Source/WebCore/editing/visible_units.cpp:1322
> +static VisiblePosition rightmostWordBoundaryInPreviousBoxes(const InlineBox*
box, int offset, TextDirection blockDirection, const VisiblePosition&
visiblePosition)

This function name is confusing because I get an impression that we start
looking for a word boundary from box->prevLeafChild().
maybe leftWordBoundary or rightmostWordBoundaryInBoxOrPreviousBoxes?

> Source/WebCore/editing/visible_units.cpp:1348
> +    // FIXME: If the box's directionality is the same as that of the
enclosing block, when the offset is at the box boundary and the direction is
towards inside the box,
> +    // do I still need to make it a special case? For example, a LTR box
inside a LTR block, when offset is at box's caretMinOffset and the direction is
DirectionRight,
> +    // should it be taken care as a general case?

These lines look too long.  Please make each line a little shorter (20-30%).

> Source/WebCore/editing/visible_units.cpp:1352
> +	   if (wordDirection == WebCore::DirectionLeft)
> +	       return rightmostWordBoundaryInPreviousBoxes(isLTRBox ?
box->prevLeafChild() : box, isLTRBox ? -1 : offset, blockDirection,
visiblePosition);
> +	   return leftmostWordBoundaryInNextBoxes(isLTRBox ? box :
box->nextLeafChild(), isLTRBox ? offset : -1, blockDirection, visiblePosition);


Maybe it's better to have these logic directly in leftWordPosition and
rightWordPosition.

e.g.
VisiblePosition leftWordPosition(const VisiblePosition& visiblePosition) {
    InlineBox* box;
    int offset;
    visiblePosition.getInlineBoxAndOffset(box, offset);
    TextDirection blockDirection =
directionOfEnclosingBlock(visiblePosition.deepEquivalent());

    if (offset == box->leftmostCaretOffset())
	return rightmostWordBoundaryInPreviousBoxes(box->prevLeafChild(), -1,
blockDirection, visiblePosition);
    else if (offset == box->rightmostCaretoffset())
	return rightmostWordBoundaryInPreviousBoxes(box, offset,
blockDirection, visiblePosition);

     // FIXME: Not implemented.
     return VisiblePosition();
}

> LayoutTests/editing/selection/move-by-word-visually-expected.txt:1
> +CONSOLE MESSAGE: line 180: TypeError: 'null' is not an object (evaluating
'document.getElementById("testGroup").style')

Do we expect to see this error?

> LayoutTests/editing/selection/move-by-word-visually-expected.txt:37
> +"abc def hij opq rst"[0, 0]
> +"abc def hij opq rst"[1, 1]
> +"abc def hij opq rst"[2, 2]
> +"abc def hij opq rst"[3, 3]
> +"abc def hij opq rst"[4, 4]
> +"abc def hij opq rst"[5, 5]
> +"abc def hij opq rst"[6, 6]
> +"abc def hij opq rst"[7, 7]
> +"abc def hij opq rst"[8, 8]
> +"abc def hij opq rst"[9, 9]
> +"abc def hij opq rst"[10, 10]
> +"abc def hij opq rst"[11, 11]
> +"abc def hij opq rst"[12, 12]
> +"abc def hij opq rst"[13, 13]
> +"abc def hij opq rst"[14, 14]
> +"abc def hij opq rst"[15, 15]
> +"abc def hij opq rst"[16, 16]
> +"abc def hij opq rst"[17, 17]
> +"abc def hij opq rst"[18, 18]
> +"abc def hij opq rst"[19, 19]

How do we know these results are correct?  it seems like we want to print
either PASS or FAIL.  Otherwise, other contributors will have no idea whether
this test is passing or failing.

> LayoutTests/editing/selection/move-by-word-visually.html:1
> +<head>

Missing DOCTYPE

> LayoutTests/editing/selection/move-by-word-visually.html:3
> +    <meta http-equiv="Content-Type" content="text/html; charset=utf-8">
> +    <style>

Let's not indent these elements.  Outdenting everything will make code simpler.


> LayoutTests/editing/selection/move-by-word-visually.html:163
> +	       while (true) {
> +		   sel.setPosition(prevNode, prevOffset);
> +		   var positions = [];
> +		   positions.push({ node: sel.anchorNode, offset:
sel.anchorOffset, point: caretCoordinates() });
> +		   sel.modify("move", direction, "-webkit-visual-word");
> +		   positions.push({ node: sel.anchorNode, offset:
sel.anchorOffset, point: caretCoordinates() });
> +		   if (direction == "right")
> +		       checkCoordinatesMovingRightDown(positions);
> +		   else
> +		       checkCoordinatesMovingLeftUp(positions);
> +		   logPositions(positions);
> +		   log("\n");
> +		   sel.setPosition(prevNode, prevOffset);
> +		   sel.modify("move", direction, "character");
> +		   if (sel.anchorNode == prevNode && sel.anchorOffset ==
prevOffset) {
> +		       break;
> +		   }
> +		   prevNode = sel.anchorNode;
> +		   prevOffset = sel.anchorOffset;
> +	       }

This function seems to be a duplicate of the code above.  We should make a
function and call it twice instead of duplicating the code.