[webkit-reviews] review denied: [Bug 61346] --webkit-visual-word: ctrl-arrow is not able to reach the boundary of line : [Attachment 102185] Proposed fix with updated test cases

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Van Lam <vanlam at google.com>'s
request for review:
Bug 61346: --webkit-visual-word: ctrl-arrow is not able to reach the boundary
of line
https://bugs.webkit.org/show_bug.cgi?id=61346

Attachment 102185: Proposed fix with updated test cases
https://bugs.webkit.org/attachment.cgi?id=102185&action=review

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


> Source/WebCore/editing/visible_units.cpp:1188
> +static VisiblePosition leftmostPositionInRTLBoxInRTLBlock(const InlineBox*
box)

You should probably add inline keyboard here.

> Source/WebCore/editing/visible_units.cpp:1218
> +	   if (box->bidiLevel() == 1) {

I don't think we should be checking that the bidi level is 1.  If anything it
should be checking that it's odd.  r- because of this.

> Source/WebCore/editing/visible_units.cpp:1257
> +    if (needToUseLogicalEndOfLine(box, blockDirection))
> +	   return
logicalEndOfLine(createPositionAvoidingIgnoredNode(box->renderer()->node(),
box->caretMaxOffset()));
> +    
> +    return endOfBoxPosition(box, blockDirection);

I don't understand the difference between logicalEndOfLine and
endOfBoxPosition.  We need to give them more descriptive name such that readers
can make sense of this code without having to read the code in those two
functions.

> Source/WebCore/editing/visible_units.cpp:1262
> +    return blockDirection == LTR ? !box->nextLeafChild() ||
box->nextLeafChild()->renderer()->isBR() : !box->prevLeafChild() ||
box->prevLeafChild()->renderer()->isBR();

Nit: I'd split this into two lines.

> Source/WebCore/editing/visible_units.cpp:1460
> +	   endOfBlock.getInlineBoxAndOffset(boxOfEndOfBlock,
offsetOfEndOfBlock);

positionIsInBox calls getInlineBoxAndOffset.  r- because of this.

> Source/WebCore/editing/visible_units.cpp:1485
> +	   endOfBlock.getInlineBoxAndOffset(boxOfEndOfBlock,
offsetOfEndOfBlock);

Ditto.