[Webkit-unassigned] [Bug 57806] continue experiment with moving caret by word in visual order

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=57806


Ryosuke Niwa <rniwa at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #88744|review?                     |review+
               Flag|                            |




--- Comment #20 from Ryosuke Niwa <rniwa at webkit.org>  2011-04-08 23:26:54 PST ---
(From update of attachment 88744)
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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.