[webkit-reviews] review granted: [Bug 57806] continue experiment with moving caret by word in visual order : [Attachment 88744] patch w/ layout test

[bugzilla-daemon at webkit.org]

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

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

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

r=me provided you address the following comments.  I'm excited for this patch!

> Source/WebCore/editing/visible_units.cpp:1292
> +    isLastWordBreakInBox = false;
> +
> +    // Given RTL box "ABC DEF" either follows a LTR box or is the first
visual box in an LTR block as an example,
> +    // the visual display of the RTL box is: "(0)J(10)I(9)H(8)
(7)F(6)E(5)D(4) (3)C(2)B(1)A(11)",
> +    // where the number in parenthesis represents offset in visiblePosition.

> +    // Start at offset 0, the first word break is at offset 3, the 2nd word
break is at offset 7, and the 3rd word break should be at offset 0.
> +    // But nextWordPosition() of offset 7 is offset 11, which should be
ignored, 
> +    // and the position at offset 0 should be manually added as the last
word break within the box.
> +    if
(positionIsVisuallyOrderedInBoxInBlockWithDifferentDirectionality(wordBreak,
box, offsetOfWordBreak))
> +	   return wordBreak;

Can we set isLastWordBreakInBox false right above the return statement? i.e.
if (positionIsVisuallyOrderedInBoxInBlockWithDifferentDirectionality(wordBreak,
box, offsetOfWordBreak)) {
    isLastWordBreakInBox = false;
    return wordBreak
}

> Source/WebCore/editing/visible_units.cpp:1326
> +	   struct WordBoundaryEntry wordBoundaryEntry(wordBreak,
offsetOfWordBreak);

You shouldn't put "struct" in front of a struct name. Please remove.

> Source/WebCore/editing/visible_units.cpp:1341
> +	       struct WordBoundaryEntry wordBoundaryEntry(wordBreak,
offsetOfWordBreak);

Ditto.

> Source/WebCore/editing/visible_units.cpp:1494
> +    int index = box->isLeftToRightDirection() ? greatestValueUnder(offset,
blockDirection == LTR, orderedWordBoundaries) :
> +						   smallestOffsetAbove(offset,
blockDirection == RTL, orderedWordBoundaries);

I don't think we normally align function calls like that.  Please do:
int index = box->isLeftToRightDirection() ? greatestValueUnder(offset,
blockDirection == LTR, orderedWordBoundaries) :
    smallestOffsetAbove(offset, blockDirection == RTL, orderedWordBoundaries);

> Source/WebCore/editing/visible_units.cpp:1534
> +    int index = box->isLeftToRightDirection() ? smallestOffsetAbove(offset,
blockDirection == LTR, orderedWordBoundaries) :
> +						   greatestValueUnder(offset,
blockDirection == RTL, orderedWordBoundaries);

Ditto.

> LayoutTests/ChangeLog:7
> +

Please explain changes happened in the expected result and what test case
you're adding.

> LayoutTests/editing/selection/move-by-word-visually-expected.txt:43
> -"ZQB abc RIG"[8, 8, 0, 0]	FAIL expected: [8, 4, 0, 0]
> +"ZQB abc RIG"[8, 0, 0, 0]	FAIL expected: [8, 4, 0, 0]
>  

I don't see new test result added here.  Did you forget to rebaseline?